Re: r314262 - Emit section information for extern variables.

2017-10-04 Thread Nico Weber via cfe-commits
That does the trick. Thanks for the quick help, everyone!

On Wed, Oct 4, 2017 at 6:18 PM, Keane, Erich <erich.ke...@intel.com> wrote:

> Elizabeth gave me a patch, submitted it with a test in r314939
>
>
>
> *From:* Andrews, Elizabeth
> *Sent:* Wednesday, October 4, 2017 1:54 PM
> *To:* Friedman, Eli <efrie...@codeaurora.org>; Keane, Erich <
> erich.ke...@intel.com>; Nico Weber <tha...@chromium.org>
> *Cc:* cfe-commits <cfe-commits@lists.llvm.org>
> *Subject:* RE: r314262 - Emit section information for extern variables.
>
>
>
> Alright. Will do. Thanks!
>
>
>
> *From:* Friedman, Eli [mailto:efrie...@codeaurora.org
> <efrie...@codeaurora.org>]
> *Sent:* Wednesday, October 4, 2017 4:51 PM
> *To:* Keane, Erich <erich.ke...@intel.com>; Andrews, Elizabeth <
> elizabeth.andr...@intel.com>; Nico Weber <tha...@chromium.org>
> *Cc:* cfe-commits <cfe-commits@lists.llvm.org>
> *Subject:* Re: r314262 - Emit section information for extern variables.
>
>
>
> 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:efrie...@codeaurora.org
> <efrie...@codeaurora.org>]
> *Sent:* Wednesday, October 4, 2017 1:46 PM
> *To:* Andrews, Elizabeth <elizabeth.andr...@intel.com>
> <elizabeth.andr...@intel.com>; Nico Weber <tha...@chromium.org>
> <tha...@chromium.org>
> *Cc:* cfe-commits <cfe-commits@lists.llvm.org>
> <cfe-commits@lists.llvm.org>; Keane, Erich <erich.ke...@intel.com>
> <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 <tha...@google.com>] *On
> Behalf Of *Nico Weber
> *Sent:* Wednesday, October 4, 2017 4:29 PM
> *To:* Keane, Erich <erich.ke...@intel.com> <erich.ke...@intel.com>
> *Cc:* Andrews, Elizabeth <elizabeth.andr...@intel.com>
> <elizabeth.andr...@intel.com>; Friedman, Eli <efrie...@codeaurora.org>
> <efrie...@codeaurora.org>; cfe-commits <cfe-commits@lists.llvm.org>
> <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>
> 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")));
>

RE: r314262 - Emit section information for extern variables.

2017-10-04 Thread Keane, Erich via cfe-commits
Elizabeth gave me a patch, submitted it with a test in r314939

From: Andrews, Elizabeth
Sent: Wednesday, October 4, 2017 1:54 PM
To: Friedman, Eli <efrie...@codeaurora.org>; Keane, Erich 
<erich.ke...@intel.com>; Nico Weber <tha...@chromium.org>
Cc: cfe-commits <cfe-commits@lists.llvm.org>
Subject: RE: r314262 - Emit section information for extern variables.

Alright. Will do. Thanks!

From: Friedman, Eli [mailto:efrie...@codeaurora.org]
Sent: Wednesday, October 4, 2017 4:51 PM
To: Keane, Erich <erich.ke...@intel.com<mailto:erich.ke...@intel.com>>; 
Andrews, Elizabeth 
<elizabeth.andr...@intel.com<mailto:elizabeth.andr...@intel.com>>; Nico Weber 
<tha...@chromium.org<mailto:tha...@chromium.org>>
Cc: cfe-commits <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>>
Subject: Re: r314262 - Emit section information for extern variables.

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:efrie...@codeaurora.org]
Sent: Wednesday, October 4, 2017 1:46 PM
To: Andrews, Elizabeth 
<elizabeth.andr...@intel.com><mailto:elizabeth.andr...@intel.com>; Nico Weber 
<tha...@chromium.org><mailto:tha...@chromium.org>
Cc: cfe-commits 
<cfe-commits@lists.llvm.org><mailto:cfe-commits@lists.llvm.org>; Keane, Erich 
<erich.ke...@intel.com><mailto: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>

RE: r314262 - Emit section information for extern variables.

2017-10-04 Thread Andrews, Elizabeth via cfe-commits
Alright. Will do. Thanks!

From: Friedman, Eli [mailto:efrie...@codeaurora.org]
Sent: Wednesday, October 4, 2017 4:51 PM
To: Keane, Erich <erich.ke...@intel.com>; Andrews, Elizabeth 
<elizabeth.andr...@intel.com>; Nico Weber <tha...@chromium.org>
Cc: cfe-commits <cfe-commits@lists.llvm.org>
Subject: Re: r314262 - Emit section information for extern variables.

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:efrie...@codeaurora.org]
Sent: Wednesday, October 4, 2017 1:46 PM
To: Andrews, Elizabeth 
<elizabeth.andr...@intel.com><mailto:elizabeth.andr...@intel.com>; Nico Weber 
<tha...@chromium.org><mailto:tha...@chromium.org>
Cc: cfe-commits 
<cfe-commits@lists.llvm.org><mailto:cfe-commits@lists.llvm.org>; Keane, Erich 
<erich.ke...@intel.com><mailto: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

Re: r314262 - Emit section information for extern variables.

2017-10-04 Thread Friedman, Eli via cfe-commits

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: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

RE: r314262 - Emit section information for extern variables.

2017-10-04 Thread Keane, Erich via cfe-commits
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?

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 c

Re: r314262 - Emit section information for extern variables.

2017-10-04 Thread Friedman, Eli via cfe-commits
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] *On Behalf Of 
*Nico Weber

*Sent:* Wednesday, October 4, 2017 4:29 PM
*To:* Keane, Erich <erich.ke...@intel.com>
*Cc:* Andrews, Elizabeth <elizabeth.andr...@intel.com>; Friedman, Eli 
<efrie...@codeaurora.org>; cfe-commits <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, Oc

RE: r314262 - Emit section information for extern variables.

2017-10-04 Thread Andrews, Elizabeth via cfe-commits
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] On Behalf Of Nico Weber
Sent: Wednesday, October 4, 2017 4:29 PM
To: Keane, Erich <erich.ke...@intel.com>
Cc: Andrews, Elizabeth <elizabeth.andr...@intel.com>; Friedman, Eli 
<efrie...@codeaurora.org>; cfe-commits <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 
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=package:chromium=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 at

RE: r314262 - Emit section information for extern variables.

2017-10-04 Thread Keane, Erich via cfe-commits
Elizabeth pointed out to me separately that the “isDeclarationADefinition” 
check ought to have prevented this from happening, so she’s taking a look I 
believe.  The usage proposal I mentioned below might be handy as well.

My fear is that this is a case where the existing functionality in Chromium 
works ‘by chance’ and isn’t terribly guaranteed by the code generation of 
either compiler.  IF this is the case, it could be a situation where this 
warning has found a bug.

As I’m not convinced that this is actually a clang bug, I would like to give 
Elizabeth a chance to poke around and make that determination first before a 
revert, if that is acceptable to you.

-Erich

From: tha...@google.com [mailto:tha...@google.com] On Behalf Of Nico Weber
Sent: Wednesday, October 4, 2017 1:29 PM
To: Keane, Erich <erich.ke...@intel.com>
Cc: Andrews, Elizabeth <elizabeth.andr...@intel.com>; Friedman, Eli 
<efrie...@codeaurora.org>; cfe-commits <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 
struct r_debug _r_debug __attribute__((nocommon, section(".r_debug")));

(There's a lengthy-ish comment for why 

Re: r314262 - Emit section information for extern variables.

2017-10-04 Thread Nico Weber via cfe-commits
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> 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] *On Behalf Of *Nico
> Weber
> *Sent:* Wednesday, October 4, 2017 1:14 PM
> *To:* Keane, Erich <erich.ke...@intel.com>
> *Cc:* Andrews, Elizabeth <elizabeth.andr...@intel.com>; Friedman, Eli <
> efrie...@codeaurora.org>; cfe-commits <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>
> 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] *On Behalf Of *Nico
> Weber
> *Sent:* Wednesday, October 4, 2017 12:59 PM
> *To:* Keane, Erich <erich.ke...@intel.com>
> *Cc:* cfe-commits <cfe-commits@lists.llvm.org>
> *Subject:* Re: r314262 - Emit section information for extern variables.
>
>
>
> Hi Erich,
>
>
>
> this breaks existing code. NaCl does this:
>
>
>
> #include 
>
> 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=package:chromium=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 t

RE: r314262 - Emit section information for extern variables.

2017-10-04 Thread Keane, Erich via cfe-commits
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] On Behalf Of Nico Weber
Sent: Wednesday, October 4, 2017 1:14 PM
To: Keane, Erich <erich.ke...@intel.com>
Cc: Andrews, Elizabeth <elizabeth.andr...@intel.com>; Friedman, Eli 
<efrie...@codeaurora.org>; cfe-commits <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 
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=package:chromium=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=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/t

Re: r314262 - Emit section information for extern variables.

2017-10-04 Thread Nico Weber via cfe-commits
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> 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] *On Behalf Of *Nico
> Weber
> *Sent:* Wednesday, October 4, 2017 12:59 PM
> *To:* Keane, Erich <erich.ke...@intel.com>
> *Cc:* cfe-commits <cfe-commits@lists.llvm.org>
> *Subject:* Re: r314262 - Emit section information for extern variables.
>
>
>
> Hi Erich,
>
>
>
> this breaks existing code. NaCl does this:
>
>
>
> #include 
>
> 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=package:chromium=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> wrote:
>
> Author: erichkeane
> Date: Tue Sep 26 16:42:34 2017
> New Revision: 314262
>
> URL: http://llvm.org/viewvc/llvm-project?rev=314262=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=314261=314262=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
>"

RE: r314262 - Emit section information for extern variables.

2017-10-04 Thread Keane, Erich via cfe-commits
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] On Behalf Of Nico Weber
Sent: Wednesday, October 4, 2017 12:59 PM
To: Keane, Erich <erich.ke...@intel.com>
Cc: cfe-commits <cfe-commits@lists.llvm.org>
Subject: Re: r314262 - Emit section information for extern variables.

Hi Erich,

this breaks existing code. NaCl does this:

#include 
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=package:chromium=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=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=314261=314262=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;
+def warn_attribute_section_on_redeclaration : Warning<
+  "section attribute is specified on redeclared variable">, InGroup;

 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=314261=314262=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())
+GV->setSection(SA->getName());
+}
+
 // Handle XCore specific ABI requirements.
 if (getTriple().getArch() == llvm::Triple::x

Re: r314262 - Emit section information for extern variables.

2017-10-04 Thread Nico Weber via cfe-commits
Hi Erich,

this breaks existing code. NaCl does this:

#include 
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=package:chromium=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> wrote:

> Author: erichkeane
> Date: Tue Sep 26 16:42:34 2017
> New Revision: 314262
>
> URL: http://llvm.org/viewvc/llvm-project?rev=314262=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=314261=314262=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;
> +def warn_attribute_section_on_redeclaration : Warning<
> +  "section attribute is specified on redeclared variable">,
> InGroup;
>
>  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=314261=314262=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())
> +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=314261=314262=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() && !Old->hasAttr()) {
> +if (auto *VD = dyn_cast(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=314261=314262=diff
> 
> ==
> --- cfe/trunk/test/Sema/attr-section.c (original)
> +++