*From:*Friedman, Eli [mailto:efrie...@codeaurora.org]
*Sent:* Wednesday, October 4, 2017 1:46 PM
*To:* Andrews, Elizabeth <elizabeth.andr...@intel.com>; Nico Weber
<tha...@chromium.org>
*Cc:* cfe-commits <cfe-commits@lists.llvm.org>; Keane, Erich
<erich.ke...@intel.com>
*Subject:* Re: r314262 - Emit section information for extern variables.
Oh, that's easy to explain; sorry, I didn't think of it when I was
reviewing.
DefinitionKind has three possible values: DeclarationOnly,
TentativeDefinition, and Definition. (Tentative definitions are C
weirdness that allows you to write "int x; int x = 10;".)
For the purpose of this warning, a TentativeDefinition should count as
a definition.
-Eli
On 10/4/2017 1:38 PM, Andrews, Elizabeth wrote:
Hello,
I just spoke to Erich. The warning isn’t supposed to be emitted
when the section attribute is specified on a definition. I’m not
sure why struct r_debug _r_debug __attribute__((nocommon,
section(".r_debug"))); failed the isThisDeclarationADefiniton
check. I need to look into it.
Thanks,
Elizabeth
*From:*tha...@google.com <mailto:tha...@google.com>
[mailto:tha...@google.com] *On Behalf Of *Nico Weber
*Sent:* Wednesday, October 4, 2017 4:29 PM
*To:* Keane, Erich <erich.ke...@intel.com>
<mailto:erich.ke...@intel.com>
*Cc:* Andrews, Elizabeth <elizabeth.andr...@intel.com>
<mailto:elizabeth.andr...@intel.com>; Friedman, Eli
<efrie...@codeaurora.org> <mailto:efrie...@codeaurora.org>;
cfe-commits <cfe-commits@lists.llvm.org>
<mailto:cfe-commits@lists.llvm.org>
*Subject:* Re: r314262 - Emit section information for extern
variables.
All I know about this code that it used to build (and still builds
with gcc) and now it doesn't, sorry :-) What that code does seems
somewhat questionable, but also somewhat useful, and it feels like
the behavior change from this change here for that code might have
been unintentional.
Your suggestion sounds reasonable to me.
On Wed, Oct 4, 2017 at 4:18 PM, Keane, Erich
<erich.ke...@intel.com <mailto:erich.ke...@intel.com>> wrote:
I saw that… I don’t see where it prevents the same change from
being made in link.h.
As I mentioned, there is a solution to make this Warning less
aggressive (in the lib/Sema/SemaDecl.cpp changes, add a
condition that Old->isUsed() before the warning. I’m
wondering if that solves your issue.
It would result in
extern struct r_debug _r_debug;
struct r_debug _r_debug __attribute__((nocommon,
section(".r_debug")));
Compiling, but :
extern struct r_debug _r_debug;
r_debug = something();
struct r_debug _r_debug __attribute__((nocommon,
section(".r_debug")));
NOT compiling (which is almost definitely a bug).
*From:*tha...@google.com
<mailto:tha...@google.com>[mailto:tha...@google.com
<mailto:tha...@google.com>] *On Behalf Of *Nico Weber
*Sent:* Wednesday, October 4, 2017 1:14 PM
*To:* Keane, Erich <erich.ke...@intel.com
<mailto:erich.ke...@intel.com>>
*Cc:* Andrews, Elizabeth <elizabeth.andr...@intel.com
<mailto:elizabeth.andr...@intel.com>>; Friedman, Eli
<efrie...@codeaurora.org <mailto:efrie...@codeaurora.org>>;
cfe-commits <cfe-commits@lists.llvm.org
<mailto:cfe-commits@lists.llvm.org>>
*Subject:* Re: r314262 - Emit section information for extern
variables.
For why, here's the comment from the code I linked to:
/*
* GDB looks for this symbol name when it cannot find
PT_DYNAMIC->DT_DEBUG.
* We don't have a PT_DYNAMIC, so it will find this. Now all
we have to do
* is arrange for this space to be filled in with the dynamic
linker's
* _r_debug contents after they're initialized. That way,
attaching GDB to
* this process or examining its core file will find the PIE
we loaded, the
* dynamic linker, and all the shared libraries, making
debugging pleasant.
*/
On Wed, Oct 4, 2017 at 4:11 PM, Keane, Erich
<erich.ke...@intel.com <mailto:erich.ke...@intel.com>> wrote:
I’ve added Elizabeth, who is the original patch author.
Hopefully she can help out here. Additionally, Eli did
the original review, so hopefully he can chime in as well.
I believe the necessity for this warning came out of the
discussion on fixing the section behavior. The issue here
is that redeclaring with a different ‘section’ can cause
some pretty nasty issues, since it isn’t terribly clear
what happens if the variable is used in the meantime.
There is 1 change that I can think of that Elizabeth and I
discussed, which was to only warn in the case where there
was a USAGE between these two redeclarations. I’m not
sure that will allow na_cl to compile, however it is my
belief that if there IS a usage between link.h:64 and
nacl_bootstrap.c:434, that this is a bug in nacl that is
simply being uncovered thanks to this new warning.
Is there a good/reasonable reason the source of nacl wants
to redeclare this with a different ‘section’?
*From:*tha...@google.com
<mailto:tha...@google.com>[mailto:tha...@google.com
<mailto:tha...@google.com>] *On Behalf Of *Nico Weber
*Sent:* Wednesday, October 4, 2017 12:59 PM
*To:* Keane, Erich <erich.ke...@intel.com
<mailto:erich.ke...@intel.com>>
*Cc:* cfe-commits <cfe-commits@lists.llvm.org
<mailto:cfe-commits@lists.llvm.org>>
*Subject:* Re: r314262 - Emit section information for
extern variables.
Hi Erich,
this breaks existing code. NaCl does this:
#include <link.h>
struct r_debug _r_debug __attribute__((nocommon,
section(".r_debug")));
(There's a lengthy-ish comment for why in
https://cs.chromium.org/chromium/src/native_client/src/trusted/service_runtime/linux/nacl_bootstrap.c?q=nacl_bootstrap.c&sq=package:chromium&dr&l=424)
link.h in e.g. the debian jessie sysroot says:
extern struct r_debug _r_debug;
After this change, clang complains:
../../native_client/src/trusted/service_runtime/linux/nacl_bootstrap.c:434:16:
error: section attribute is specified on redeclared
variable [-Werror,-Wsection]
struct r_debug _r_debug __attribute__((nocommon,
section(".r_debug")));
^
../../build/linux/debian_jessie_amd64-sysroot/usr/include/link.h:67:23:
note: previous declaration is here
extern struct r_debug _r_debug;
This code used to work in clang, and continues to work in
gcc. So this patch probably isn't quite the right
approach. Ideas?
On Tue, Sep 26, 2017 at 7:42 PM, Erich Keane via
cfe-commits <cfe-commits@lists.llvm.org
<mailto:cfe-commits@lists.llvm.org>> wrote:
Author: erichkeane
Date: Tue Sep 26 16:42:34 2017
New Revision: 314262
URL:
http://llvm.org/viewvc/llvm-project?rev=314262&view=rev
Log:
Emit section information for extern variables.
Currently, if _attribute_((section())) is used for
extern variables,
section information is not emitted in generated IR
when the variables are used.
This is expected since sections are not generated for
external linkage objects.
However NiosII requires this information as it uses
special GP-relative accesses
for any objects that use attribute section (.sdata).
GCC keeps this attribute in
middle-end.
This change emits the section information for all targets.
Patch By: Elizabeth Andrews
Differential Revision:https://reviews.llvm.org/D36487
Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/CodeGen/CodeGenModule.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/test/Sema/attr-section.c
Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=314262&r1=314261&r2=314262&view=diff
==============================================================================
---
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
(original)
+++
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
Tue Sep 26 16:42:34 2017
@@ -2620,6 +2620,8 @@ def
err_attribute_section_invalid_for_ta
"argument to 'section' attribute is not valid for
this target: %0">;
def warn_mismatched_section : Warning<
"section does not match previous declaration">,
InGroup<Section>;
+def warn_attribute_section_on_redeclaration : Warning<
+ "section attribute is specified on redeclared
variable">, InGroup<Section>;
def err_anonymous_property: Error<
"anonymous property is not supported">;
Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=314262&r1=314261&r2=314262&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Tue Sep 26
16:42:34 2017
@@ -2432,6 +2432,12 @@
CodeGenModule::GetOrCreateLLVMGlobal(Str
EmitGlobalVarDefinition(D);
}
+ // Emit section information for extern variables.
+ if (D->hasExternalStorage()) {
+ if (const SectionAttr *SA =
D->getAttr<SectionAttr>())
+ GV->setSection(SA->getName());
+ }
+
// Handle XCore specific ABI requirements.
if (getTriple().getArch() == llvm::Triple::xcore &&
D->getLanguageLinkage() == CLanguageLinkage &&
Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=314262&r1=314261&r2=314262&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDecl.cpp Tue Sep 26
16:42:34 2017
@@ -2607,6 +2607,16 @@ void
Sema::mergeDeclAttributes(NamedDecl
}
}
+ // This redeclaration adds a section attribute.
+ if (New->hasAttr<SectionAttr>() &&
!Old->hasAttr<SectionAttr>()) {
+ if (auto *VD = dyn_cast<VarDecl>(New)) {
+ if (VD->isThisDeclarationADefinition() !=
VarDecl::Definition) {
+ Diag(New->getLocation(),
diag::warn_attribute_section_on_redeclaration);
+ Diag(Old->getLocation(),
diag::note_previous_declaration);
+ }
+ }
+ }
+
if (!Old->hasAttrs())
return;
Modified: cfe/trunk/test/Sema/attr-section.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/attr-section.c?rev=314262&r1=314261&r2=314262&view=diff
==============================================================================
--- cfe/trunk/test/Sema/attr-section.c (original)
+++ cfe/trunk/test/Sema/attr-section.c Tue Sep 26
16:42:34 2017
@@ -19,3 +19,7 @@ void __attribute__((section("foo,zed")))
void __attribute__((section("bar,zed"))) test2(void)
{} // expected-warning {{section does not match
previous declaration}}
enum __attribute__((section("NEAR,x"))) e { one }; //
expected-error {{'section' attribute only applies to
functions, methods, properties, and global variables}}
+
+extern int a; // expected-note {{previous declaration
is here}}
+int *b = &a;
+extern int a __attribute__((section("foo,zed"))); //
expected-warning {{section attribute is specified on
redeclared variable}}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
<mailto:cfe-commits@lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
Foundation Collaborative Project
