r314262 - Emit section information for extern variables.
Friedman, Eli via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 4 13:51:07 PDT 2017
On 10/4/2017 1:48 PM, Keane, Erich wrote:
>
> Ah, cool! I didn’t realize that, and that is actually exactly what
> Elizabeth is looking into now.
>
> Elizabeth, if you send me a diff, I’ll commit it as
> review-after-commit if Eli is alright with this. It should be
> something like changing:
>
> + if (VD->isThisDeclarationADefinition() != VarDecl::Definition) {
>
> To
>
> + if (VD->isThisDeclarationADefinition() != VarDecl::Definition
> && VD->isThsDeclarationADefinition() != VarDecl::TentativeDefinition) {
>
> Right?
>
I'd probably just write it "if (VD->isThisDeclarationADefinition() ==
VarDecl::DeclarationOnly)", but yes. (And don't forget a testcase.)
-Eli
> *From:*Friedman, Eli [mailto:efriedma at codeaurora.org]
> *Sent:* Wednesday, October 4, 2017 1:46 PM
> *To:* Andrews, Elizabeth <elizabeth.andrews at intel.com>; Nico Weber
> <thakis at chromium.org>
> *Cc:* cfe-commits <cfe-commits at lists.llvm.org>; Keane, Erich
> <erich.keane at 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:*thakis at google.com <mailto: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>
> <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.
>
> 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
--
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/fb99423d/attachment-0001.html>
More information about the cfe-commits
mailing list