r314262 - Emit section information for extern variables.
Nico Weber via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 4 13:29:08 PDT 2017
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> 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] *On Behalf Of *Nico
> Weber
> *Sent:* Wednesday, October 4, 2017 1:14 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.
>
>
>
> 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>
> 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] *On Behalf Of *Nico
> Weber
> *Sent:* Wednesday, October 4, 2017 12:59 PM
> *To:* Keane, Erich <erich.keane at intel.com>
> *Cc:* cfe-commits <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> 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
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171004/53c4b1c7/attachment-0001.html>
More information about the cfe-commits
mailing list