r314262 - Emit section information for extern variables.
Friedman, Eli via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 4 13:45:46 PDT 2017
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:*thakis at google.com [mailto:thakis at google.com] *On Behalf Of
> *Nico Weber
> *Sent:* Wednesday, October 4, 2017 4:29 PM
> *To:* Keane, Erich <erich.keane at intel.com>
> *Cc:* Andrews, Elizabeth <elizabeth.andrews at intel.com>; Friedman, Eli
> <efriedma at codeaurora.org>; cfe-commits <cfe-commits at 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.keane at intel.com
> <mailto:erich.keane at 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:*thakis at google.com
> <mailto:thakis at google.com>[mailto:thakis at google.com
> <mailto:thakis at google.com>] *On Behalf Of *Nico Weber
> *Sent:* Wednesday, October 4, 2017 1:14 PM
> *To:* Keane, Erich <erich.keane at intel.com
> <mailto:erich.keane at intel.com>>
> *Cc:* Andrews, Elizabeth <elizabeth.andrews at intel.com
> <mailto:elizabeth.andrews at intel.com>>; Friedman, Eli
> <efriedma at codeaurora.org <mailto:efriedma at codeaurora.org>>;
> cfe-commits <cfe-commits at lists.llvm.org
> <mailto:cfe-commits at 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.keane at intel.com <mailto:erich.keane at 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:*thakis at google.com
> <mailto:thakis at google.com>[mailto:thakis at google.com
> <mailto:thakis at google.com>] *On Behalf Of *Nico Weber
> *Sent:* Wednesday, October 4, 2017 12:59 PM
> *To:* Keane, Erich <erich.keane at intel.com
> <mailto:erich.keane at intel.com>>
> *Cc:* cfe-commits <cfe-commits at lists.llvm.org
> <mailto:cfe-commits at 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 at lists.llvm.org
> <mailto:cfe-commits at 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 at lists.llvm.org <mailto:cfe-commits at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171004/73e7f8fb/attachment-0001.html>
More information about the cfe-commits
mailing list