[PATCH] proposed fix for bug # 61144

[PATCH] proposed fix for bug # 61144

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

Bug # 61144 is a regression in 4.9.0 that breaks building of musl libc
due to aggressive and semantically-incorrect constant folding of weak
aliases. The attached patch seems to fix the issue. A weak alias
should never be a candidate for constant folding because it may always
be replaced by a strong definition from another translation unit.

For details see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61144

I do not have a copyright assignment on file but this patch should be
sufficiently trivial not to require it.

Rich

[-- Attachment #2: pr61144-v1.diff --]
[-- Type: text/plain, Size: 429 bytes --]

diff --git a/gcc/varpool.c b/gcc/varpool.c
index b426757..905047e 100644
--- a/gcc/varpool.c
+++ b/gcc/varpool.c
@@ -195,6 +195,8 @@ ctor_for_folding (tree decl)
     {
       gcc_assert (!DECL_INITIAL (decl)
 		  || DECL_INITIAL (decl) == error_mark_node);
+      if (DECL_WEAK (decl))
+        return error_mark_node;
       if (lookup_attribute ("weakref", DECL_ATTRIBUTES (decl)))
 	{
 	  node = varpool_alias_target (node);

Re: [PATCH] proposed fix for bug # 61144

[Richard Biener]

On Wed, May 21, 2014 at 3:59 AM, Rich Felker <dalias@libc.org> wrote:
> Bug # 61144 is a regression in 4.9.0 that breaks building of musl libc
> due to aggressive and semantically-incorrect constant folding of weak
> aliases. The attached patch seems to fix the issue. A weak alias
> should never be a candidate for constant folding because it may always
> be replaced by a strong definition from another translation unit.
>
> For details see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61144
>
> I do not have a copyright assignment on file but this patch should be
> sufficiently trivial not to require it.

Please add a testcase.  Also I wonder why it isn't better to generalize

  /* Variables declared 'const' without an initializer
     have zero as the initializer if they may not be
     overridden at link or run time.  */
  if (!DECL_INITIAL (real_decl)
      && (DECL_EXTERNAL (decl) || decl_replaceable_p (decl)))
    return error_mark_node;

Honza?

Thanks,
Richard.



> Rich

Re: [PATCH] proposed fix for bug # 61144

[Rich Felker]

On Wed, May 21, 2014 at 11:17:53AM +0200, Richard Biener wrote:
> On Wed, May 21, 2014 at 3:59 AM, Rich Felker <dalias@libc.org> wrote:
> > Bug # 61144 is a regression in 4.9.0 that breaks building of musl libc
> > due to aggressive and semantically-incorrect constant folding of weak
> > aliases. The attached patch seems to fix the issue. A weak alias
> > should never be a candidate for constant folding because it may always
> > be replaced by a strong definition from another translation unit.
> >
> > For details see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61144
> >
> > I do not have a copyright assignment on file but this patch should be
> > sufficiently trivial not to require it.
> 
> Please add a testcase.  Also I wonder why it isn't better to generalize

How should a testcase be done? On the PR there's a testcase that shows
the problem in the generated code, but no automated check for it.
Testing this is actually a bit of a pain unless you're allowed to run
the generated program.

>   /* Variables declared 'const' without an initializer
>      have zero as the initializer if they may not be
>      overridden at link or run time.  */
>   if (!DECL_INITIAL (real_decl)
>       && (DECL_EXTERNAL (decl) || decl_replaceable_p (decl)))
>     return error_mark_node;
> 
> Honza?

Indeed, this may be a better place to do it as long as
decl_replaceable_p reliably returns true for weak aliases. If so, the
following might work:

   if ((!DECL_INITIAL (real_decl) && DECL_EXTERNAL (decl))
       || decl_replaceable_p (decl)))
     return error_mark_node;

On the other hand, I might just separate it out into two separate if
statements since they should probably have their own comments.

I would appreciate help from anyone familiar with GCC internals on
getting this right.

Rich

Re: [PATCH] proposed fix for bug # 61144

[Jeff Law]

On 05/21/14 21:59, Rich Felker wrote:
> On Wed, May 21, 2014 at 11:17:53AM +0200, Richard Biener wrote:
>> On Wed, May 21, 2014 at 3:59 AM, Rich Felker <dalias@libc.org> wrote:
>>> Bug # 61144 is a regression in 4.9.0 that breaks building of musl libc
>>> due to aggressive and semantically-incorrect constant folding of weak
>>> aliases. The attached patch seems to fix the issue. A weak alias
>>> should never be a candidate for constant folding because it may always
>>> be replaced by a strong definition from another translation unit.
>>>
>>> For details see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61144
>>>
>>> I do not have a copyright assignment on file but this patch should be
>>> sufficiently trivial not to require it.
>>
>> Please add a testcase.  Also I wonder why it isn't better to generalize
>
> How should a testcase be done? On the PR there's a testcase that shows
> the problem in the generated code, but no automated check for it.
> Testing this is actually a bit of a pain unless you're allowed to run
> the generated program.
You can run the test program.  Have it exit (0) on success, abort () on 
failure if at all possible.  Then drop the test source file into 
gcc/testsuite/gcc.c-torture/execute/pr61144.c

At least that's the easiest way when you don't need to pass special 
arguments, restrict it to certain targets, etc.

Jeff

Re: [PATCH] proposed fix for bug # 61144

[Rich Felker]

On Fri, May 23, 2014 at 12:26:18PM -0600, Jeff Law wrote:
> On 05/21/14 21:59, Rich Felker wrote:
> >On Wed, May 21, 2014 at 11:17:53AM +0200, Richard Biener wrote:
> >>On Wed, May 21, 2014 at 3:59 AM, Rich Felker <dalias@libc.org> wrote:
> >>>Bug # 61144 is a regression in 4.9.0 that breaks building of musl libc
> >>>due to aggressive and semantically-incorrect constant folding of weak
> >>>aliases. The attached patch seems to fix the issue. A weak alias
> >>>should never be a candidate for constant folding because it may always
> >>>be replaced by a strong definition from another translation unit.
> >>>
> >>>For details see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61144
> >>>
> >>>I do not have a copyright assignment on file but this patch should be
> >>>sufficiently trivial not to require it.
> >>
> >>Please add a testcase.  Also I wonder why it isn't better to generalize
> >
> >How should a testcase be done? On the PR there's a testcase that shows
> >the problem in the generated code, but no automated check for it.
> >Testing this is actually a bit of a pain unless you're allowed to run
> >the generated program.
> You can run the test program.  Have it exit (0) on success, abort ()
> on failure if at all possible.  Then drop the test source file into
> gcc/testsuite/gcc.c-torture/execute/pr61144.c

The test needs to be two translation units linked together: one with
a weak definition of the object as 0, and the other with a strong
definition. The test should show the weak value being used rather than
the strong one. But I'm not sure how this should be integrated with
the build process.

Is anyone from the gcc side willing to help on this? I'd really like
to get it fixed before the bug propagates into more releases and makes
longstanding weak alias semantics unreliable, but I'm not familiar
with gcc internals well enough to know if my existing patch is the
preferred fix or not, nor how to make a good test case.

Rich

Re: [PATCH] proposed fix for bug # 61144

[Alexander Monakov]

On Fri, 6 Jun 2014, Rich Felker wrote:

> On Fri, May 23, 2014 at 12:26:18PM -0600, Jeff Law wrote:
> > On 05/21/14 21:59, Rich Felker wrote:
> > >On Wed, May 21, 2014 at 11:17:53AM +0200, Richard Biener wrote:
> > >>On Wed, May 21, 2014 at 3:59 AM, Rich Felker <dalias@libc.org> wrote:
> > >>>Bug # 61144 is a regression in 4.9.0 that breaks building of musl libc
> > >>>due to aggressive and semantically-incorrect constant folding of weak
> > >>>aliases. The attached patch seems to fix the issue. A weak alias
> > >>>should never be a candidate for constant folding because it may always
> > >>>be replaced by a strong definition from another translation unit.
> > >>>
> > >>>For details see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61144
> > >>>
> > >>>I do not have a copyright assignment on file but this patch should be
> > >>>sufficiently trivial not to require it.
> > >>
> > >>Please add a testcase.  Also I wonder why it isn't better to generalize
> > >
> > >How should a testcase be done? On the PR there's a testcase that shows
> > >the problem in the generated code, but no automated check for it.
> > >Testing this is actually a bit of a pain unless you're allowed to run
> > >the generated program.
> > You can run the test program.  Have it exit (0) on success, abort ()
> > on failure if at all possible.  Then drop the test source file into
> > gcc/testsuite/gcc.c-torture/execute/pr61144.c
> 
> The test needs to be two translation units linked together: one with
> a weak definition of the object as 0, and the other with a strong
> definition. The test should show the weak value being used rather than
> the strong one. But I'm not sure how this should be integrated with
> the build process.

Please have a look at gcc/testsuite/gcc.dg/special/wkali-2{,a,b}.c.  This is a
three-TU test for weak aliases, so you should be able to very easily adjust it
for this bug.

Thanks.
Alexander

Re: [PATCH] proposed fix for bug # 61144

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 2081 bytes --]

On Mon, Jun 09, 2014 at 03:40:44PM +0400, Alexander Monakov wrote:
> 
> 
> On Fri, 6 Jun 2014, Rich Felker wrote:
> 
> > On Fri, May 23, 2014 at 12:26:18PM -0600, Jeff Law wrote:
> > > On 05/21/14 21:59, Rich Felker wrote:
> > > >On Wed, May 21, 2014 at 11:17:53AM +0200, Richard Biener wrote:
> > > >>On Wed, May 21, 2014 at 3:59 AM, Rich Felker <dalias@libc.org> wrote:
> > > >>>Bug # 61144 is a regression in 4.9.0 that breaks building of musl libc
> > > >>>due to aggressive and semantically-incorrect constant folding of weak
> > > >>>aliases. The attached patch seems to fix the issue. A weak alias
> > > >>>should never be a candidate for constant folding because it may always
> > > >>>be replaced by a strong definition from another translation unit.
> > > >>>
> > > >>>For details see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61144
> > > >>>
> > > >>>I do not have a copyright assignment on file but this patch should be
> > > >>>sufficiently trivial not to require it.
> > > >>
> > > >>Please add a testcase.  Also I wonder why it isn't better to generalize
> > > >
> > > >How should a testcase be done? On the PR there's a testcase that shows
> > > >the problem in the generated code, but no automated check for it.
> > > >Testing this is actually a bit of a pain unless you're allowed to run
> > > >the generated program.
> > > You can run the test program.  Have it exit (0) on success, abort ()
> > > on failure if at all possible.  Then drop the test source file into
> > > gcc/testsuite/gcc.c-torture/execute/pr61144.c
> > 
> > The test needs to be two translation units linked together: one with
> > a weak definition of the object as 0, and the other with a strong
> > definition. The test should show the weak value being used rather than
> > the strong one. But I'm not sure how this should be integrated with
> > the build process.
> 
> Please have a look at gcc/testsuite/gcc.dg/special/wkali-2{,a,b}.c.  This is a
> three-TU test for weak aliases, so you should be able to very easily adjust it
> for this bug.

Are the attached files acceptable?

Rich

[-- Attachment #2: wkali-3.c --]
[-- Type: text/plain, Size: 307 bytes --]

/* { dg-do run } */
/* { dg-require-weak "" } */
/* { dg-require-alias "" } */
/* { dg-additional-sources "wkali-3a.c" } */

#include <stdlib.h>

static int dummy = 0;
extern int foo __attribute__((__weak__, __alias__("dummy")));

int main(void) {

    if (foo)
        exit(0);
    else
        abort();
}

[-- Attachment #3: wkali-3a.c --]
[-- Type: text/plain, Size: 34 bytes --]

/* { dg-do run } */

int foo = 1;

Re: [PATCH] proposed fix for bug # 61144

[Jan Hubicka]

> 
> Are the attached files acceptable?

The testcase looks OK to me, but it already should be fixed on mainline
by patch https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01315.html that
prevents dummy to be marked as constant. 

You can however modify the testcase to have
__attribute__ ((weak)) const int foo=0;

This needs your decl_replaceable change to not be optimized to if (0),
because of the explicit const modifier.

I did not change ctor_for_folding to reject variables above as I was not quite
sure we want to support this kind of interposition and I am still not quite certain.
C++ is quite clear about the transformation replacing initialized const by its value.

Honza

> 
> Rich

> /* { dg-do run } */
> /* { dg-require-weak "" } */
> /* { dg-require-alias "" } */
> /* { dg-additional-sources "wkali-3a.c" } */
> 
> #include <stdlib.h>
> 
> static int dummy = 0;
> extern int foo __attribute__((__weak__, __alias__("dummy")));
> 
> int main(void) {
> 
>     if (foo)
>         exit(0);
>     else
>         abort();
> }

> /* { dg-do run } */
> 
> int foo = 1;

Re: [PATCH] proposed fix for bug # 61144

[Rich Felker]

On Mon, Jun 16, 2014 at 11:06:04AM +0200, Jan Hubicka wrote:
> > 
> > Are the attached files acceptable?
> 
> The testcase looks OK to me, but it already should be fixed on mainline
> by patch https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01315.html that
> prevents dummy to be marked as constant. 
> 
> You can however modify the testcase to have
> __attribute__ ((weak)) const int foo=0;

And the same for weak alias rather than straight weak definition like
the above?

> This needs your decl_replaceable change to not be optimized to if (0),
> because of the explicit const modifier.

The case I care about actually has "dummy" as const (with the intent
that it be allocated in a read-only section if the dummy definition is
used). So for me it's important that this regression be fixed too.

> I did not change ctor_for_folding to reject variables above as I was not quite
> sure we want to support this kind of interposition and I am still not quite certain.
> C++ is quite clear about the transformation replacing initialized const by its value.

My concern is about C, not C++. This kind of interposition has always
been supported in unix C, even prior to GCC, going back to sysv or
earlier, as a documented feature (historically #pragma weak). It
should not regress. If fixing it results in an regression with regards
to optimizability of C++, perhaps this could be made
language-specific, or (better) the C++ front-end could add an
additional internal-use-only attribute to weak definitions it
generates internally that permits constant-folding them, while not
breaking the semantics for weak definitions provided by the user at
the source level.

Rich

Re: [PATCH] proposed fix for bug # 61144

[Jan Hubicka]

> On Mon, Jun 16, 2014 at 11:06:04AM +0200, Jan Hubicka wrote:
> > > 
> > > Are the attached files acceptable?
> > 
> > The testcase looks OK to me, but it already should be fixed on mainline
> > by patch https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01315.html that
> > prevents dummy to be marked as constant. 
> > 
> > You can however modify the testcase to have
> > __attribute__ ((weak)) const int foo=0;
> 
> And the same for weak alias rather than straight weak definition like
> the above?

Yes, if you add const to your testcase, it will get miscompiled by mainline
again. 
> 
> > This needs your decl_replaceable change to not be optimized to if (0),
> > because of the explicit const modifier.
> 
> The case I care about actually has "dummy" as const (with the intent
> that it be allocated in a read-only section if the dummy definition is
> used). So for me it's important that this regression be fixed too.

Yep, GCC since 90's was optimizing reads from weak const attributes, but it
because worse because I added code walking through aliases.
> 
> > I did not change ctor_for_folding to reject variables above as I was not quite
> > sure we want to support this kind of interposition and I am still not quite certain.
> > C++ is quite clear about the transformation replacing initialized const by its value.
> 
> My concern is about C, not C++. This kind of interposition has always
> been supported in unix C, even prior to GCC, going back to sysv or
> earlier, as a documented feature (historically #pragma weak). It
> should not regress. If fixing it results in an regression with regards
> to optimizability of C++, perhaps this could be made
> language-specific, or (better) the C++ front-end could add an
> additional internal-use-only attribute to weak definitions it
> generates internally that permits constant-folding them, while not
> breaking the semantics for weak definitions provided by the user at
> the source level.

Yes, I see your point and clearly we should not optimize with explicit weak attribute.
I wonder if decl_replaceable_p is however correct check here or if we want explicit check
for weak visibility.

I am concerned about const variables w/o weak attribute with -fPIC (because for
those decl_replaceable_p returns true, too). Consider following testcase:
struct t
{
static const int dummy=0;
const int *m();
} t;
int
main()
{
  return *t.m();
}
int
main2()
{
  return t.dummy;
}
const int *
t::m()
{
  return &dummy;
}

Here main2 is optimized by C++ FE to return 0, while backend is affraid to optimize
main() after inlining anticipating that dummy may be interposed. However moving t::m
inside of the unit will make dummy comdat and it will get optimizing.
Adding another method and keying the t into other unit will make it optimized, too.

This is not very consistent. But perhaps we need a flag from C++ FE to tell us
what variables may not be interposed, because perhaps the c variant with -fPIC
const int dummy=0;
int
main()
{
  return t;
}

Jason?

A C variant of the testcase:

const int dummy=0;

const static int * d=&dummy;
int
main()
{
  return dummy;
}
int
main2()
{
  return *d;
}

seems optimized to return 0 (with -fPIC) for ages, too, but here at least
frontend won't substitute first dummy for 0.

Honza

Re: [PATCH] proposed fix for bug # 61144

[Rich Felker]

On Mon, Jun 16, 2014 at 06:05:19PM +0200, Jan Hubicka wrote:
> > > This needs your decl_replaceable change to not be optimized to if (0),
> > > because of the explicit const modifier.
> > 
> > The case I care about actually has "dummy" as const (with the intent
> > that it be allocated in a read-only section if the dummy definition is
> > used). So for me it's important that this regression be fixed too.
> 
> Yep, GCC since 90's was optimizing reads from weak const attributes, but it
> because worse because I added code walking through aliases.

Ah, yes. BTW the reason I've avoided weak references and straight-out
weak definitions in favor of using weak aliases (aside from aliases
saving space in some cases) is that weak-alias seems to be the
baseline feature that's "always worked safely" whereas the others have
had bugs on and off in the past. And I really hope we can avoid having
more than a single release where it's broken. Some distros are already
pulling in 4.9.0 and I'm getting more bug reports.

> > > I did not change ctor_for_folding to reject variables above as I was not quite
> > > sure we want to support this kind of interposition and I am still not quite certain.
> > > C++ is quite clear about the transformation replacing initialized const by its value.
> > 
> > My concern is about C, not C++. This kind of interposition has always
> > been supported in unix C, even prior to GCC, going back to sysv or
> > earlier, as a documented feature (historically #pragma weak). It
> > should not regress. If fixing it results in an regression with regards
> > to optimizability of C++, perhaps this could be made
> > language-specific, or (better) the C++ front-end could add an
> > additional internal-use-only attribute to weak definitions it
> > generates internally that permits constant-folding them, while not
> > breaking the semantics for weak definitions provided by the user at
> > the source level.
> 
> Yes, I see your point and clearly we should not optimize with explicit weak attribute.
> I wonder if decl_replaceable_p is however correct check here or if we want explicit check
> for weak visibility.

Isn't anything weak inherently replacable? It seems like using
decl_replacable_p, if that predicate is correctly implemented, would
be completely safe, since it should be true for a superset of weak.

> I am concerned about const variables w/o weak attribute with -fPIC (because for
> those decl_replaceable_p returns true, too). Consider following testcase:
> struct t
> {
> static const int dummy=0;
> const int *m();
> } t;
> int
> main()
> {
>   return *t.m();
> }
> int
> main2()
> {
>   return t.dummy;
> }
> const int *
> t::m()
> {
>   return &dummy;
> }
> 
> Here main2 is optimized by C++ FE to return 0, while backend is affraid to optimize
> main() after inlining anticipating that dummy may be interposed. However moving t::m
> inside of the unit will make dummy comdat and it will get optimizing.
> Adding another method and keying the t into other unit will make it optimized, too.
> 
> This is not very consistent. But perhaps we need a flag from C++ FE to tell us
> what variables may not be interposed, because perhaps the c variant with -fPIC
> const int dummy=0;
> int
> main()
> {
>   return t;
> }
> 
> Jason?
> 
> A C variant of the testcase:
> 
> const int dummy=0;
> 
> const static int * d=&dummy;
> int
> main()
> {
>   return dummy;
> }
> int
> main2()
> {
>   return *d;
> }
> 
> seems optimized to return 0 (with -fPIC) for ages, too, but here at least
> frontend won't substitute first dummy for 0.

I see the issue of whether interposition should be supported in
non-weak situations like this as completely separate from pr61144.
Strictly speaking, it's undefined behavior to have multiple
definitions of the same symbol. ELF semantics allow such interposition
for shared library use mainly because creating a shared library hides
translation-unit boundaries whereby symbol replacement with the
equivalent static library may have had well-defined behavior due to
the conflicting translation units not getting pulled in by the linker.
But there's no fundamental reason to need to support interposition
against symbols defined in the same translation unit, since in the
corresponding static-library usage it would result in a link-time
error.

With the weak symbols issue (pr61144), on the other hand, the bug is
that the well-established semantics of weak binding are not being
honored by the compiler.

Of course it may still be _desirable_ to support the sort of
interposition you described for definitions in the same translation
unit, but whether you want to support that should be treated as a
separate issue IMO, for the above reasons.

Rich

Re: [PATCH] proposed fix for bug # 61144

[Rich Felker]

Ping. Do you have any feedback on my tests? What is the next step?

Rich

On Mon, Jun 09, 2014 at 03:40:44PM +0400, Alexander Monakov wrote:
> 
> 
> On Fri, 6 Jun 2014, Rich Felker wrote:
> 
> > On Fri, May 23, 2014 at 12:26:18PM -0600, Jeff Law wrote:
> > > On 05/21/14 21:59, Rich Felker wrote:
> > > >On Wed, May 21, 2014 at 11:17:53AM +0200, Richard Biener wrote:
> > > >>On Wed, May 21, 2014 at 3:59 AM, Rich Felker <dalias@libc.org> wrote:
> > > >>>Bug # 61144 is a regression in 4.9.0 that breaks building of musl libc
> > > >>>due to aggressive and semantically-incorrect constant folding of weak
> > > >>>aliases. The attached patch seems to fix the issue. A weak alias
> > > >>>should never be a candidate for constant folding because it may always
> > > >>>be replaced by a strong definition from another translation unit.
> > > >>>
> > > >>>For details see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61144
> > > >>>
> > > >>>I do not have a copyright assignment on file but this patch should be
> > > >>>sufficiently trivial not to require it.
> > > >>
> > > >>Please add a testcase.  Also I wonder why it isn't better to generalize
> > > >
> > > >How should a testcase be done? On the PR there's a testcase that shows
> > > >the problem in the generated code, but no automated check for it.
> > > >Testing this is actually a bit of a pain unless you're allowed to run
> > > >the generated program.
> > > You can run the test program.  Have it exit (0) on success, abort ()
> > > on failure if at all possible.  Then drop the test source file into
> > > gcc/testsuite/gcc.c-torture/execute/pr61144.c
> > 
> > The test needs to be two translation units linked together: one with
> > a weak definition of the object as 0, and the other with a strong
> > definition. The test should show the weak value being used rather than
> > the strong one. But I'm not sure how this should be integrated with
> > the build process.
> 
> Please have a look at gcc/testsuite/gcc.dg/special/wkali-2{,a,b}.c.  This is a
> three-TU test for weak aliases, so you should be able to very easily adjust it
> for this bug.
> 
> Thanks.
> Alexander

Re: [PATCH] proposed fix for bug # 61144

[Jan Hubicka]

> >   /* Variables declared 'const' without an initializer
> >      have zero as the initializer if they may not be
> >      overridden at link or run time.  */
> >   if (!DECL_INITIAL (real_decl)
> >       && (DECL_EXTERNAL (decl) || decl_replaceable_p (decl)))
> >     return error_mark_node;
> > 
> > Honza?
> 
> Indeed, this may be a better place to do it as long as
> decl_replaceable_p reliably returns true for weak aliases. If so, the
> following might work:
> 
>    if ((!DECL_INITIAL (real_decl) && DECL_EXTERNAL (decl))
>        || decl_replaceable_p (decl)))
>      return error_mark_node;
> 
> On the other hand, I might just separate it out into two separate if
> statements since they should probably have their own comments.

Yep, this looks like correct change.  I used to have FIXME on this but
it seems it went away during some cleanups - the original condition was
comming from expmed's folding and indeed it looked unsafe to me.

This change is OK with the testcase (if it passes testing)

Honza
> 
> I would appreciate help from anyone familiar with GCC internals on
> getting this right.
> 
> Rich

Re: [PATCH] proposed fix for bug # 61144

[Alexander Monakov]

On Mon, 16 Jun 2014, Jan Hubicka wrote:
> > >   /* Variables declared 'const' without an initializer
> > >      have zero as the initializer if they may not be
> > >      overridden at link or run time.  */
> > >   if (!DECL_INITIAL (real_decl)
> > >       && (DECL_EXTERNAL (decl) || decl_replaceable_p (decl)))
> > >     return error_mark_node;
> > > 
> > > Honza?
> > 
> > Indeed, this may be a better place to do it as long as
> > decl_replaceable_p reliably returns true for weak aliases. If so, the
> > following might work:
> > 
> >    if ((!DECL_INITIAL (real_decl) && DECL_EXTERNAL (decl))
> >        || decl_replaceable_p (decl)))
> >      return error_mark_node;
> > 
> > On the other hand, I might just separate it out into two separate if
> > statements since they should probably have their own comments.
> 
> Yep, this looks like correct change.  I used to have FIXME on this but
> it seems it went away during some cleanups - the original condition was
> comming from expmed's folding and indeed it looked unsafe to me.
> 
> This change is OK with the testcase (if it passes testing)

I'd like to push this topic forward a bit.  I've bootstrapped and regtested a
version of the patch based on the initial proposal to check DECL_WEAK.  The
approach with decl_replaceable_p looks not that easy; I'll expand in a
followup email.

OK for trunk/branch?

2014-07-22  Rich Felker  <dalias@libc.org>
	    Alexander Monakov  <amonakov@ispras.ru>

gcc/
	PR ipa/61144
	* varpool.c (ctor_for_folding): Reject weak data.

gcc/testsuite/

	PR ipa/61144
	* gcc.dg/special/wkali-3.c: New test.
	* gcc.dg/special/wkali-3a.c: Auxiliary file.

diff --git a/gcc/varpool.c b/gcc/varpool.c
index 04ce714..9ef2195 100644
--- a/gcc/varpool.c
+++ b/gcc/varpool.c
@@ -378,6 +378,9 @@ ctor_for_folding (tree decl)
       return error_mark_node;
     }
 
+  if (DECL_WEAK (decl) && !DECL_VIRTUAL_P (decl))
+    return error_mark_node;
+
   gcc_assert (TREE_CODE (decl) == VAR_DECL);
 
   real_node = node = varpool_get_node (decl);
diff --git a/gcc/testsuite/gcc.dg/special/wkali-3.c b/gcc/testsuite/gcc.dg/special/wkali-3.c
new file mode 100644
index 0000000..407ace6
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/special/wkali-3.c
@@ -0,0 +1,17 @@
+/* { dg-do run } */
+/* { dg-require-weak "" } */
+/* { dg-require-alias "" } */
+/* { dg-additional-sources "wkali-3a.c" } */
+
+#include <stdlib.h>
+
+static const int dummy = 0;
+extern const int foo __attribute__((__weak__, __alias__("dummy")));
+
+int main(void) {
+
+    if (foo)
+        exit(0);
+    else
+        abort();
+}
diff --git a/gcc/testsuite/gcc.dg/special/wkali-3a.c b/gcc/testsuite/gcc.dg/special/wkali-3a.c
new file mode 100644
index 0000000..b3bc6a4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/special/wkali-3a.c
@@ -0,0 +1,3 @@
+/* { dg-do run } */
+
+int foo = 1;

Re: [PATCH] proposed fix for bug # 61144

[Alexander Monakov]

On Tue, 22 Jul 2014, Alexander Monakov wrote:
> I'd like to push this topic forward a bit.  I've bootstrapped and regtested a
> version of the patch based on the initial proposal to check DECL_WEAK.  The
> approach with decl_replaceable_p looks not that easy; I'll expand in a
> followup email.

The problem with the patch below using decl_replaceable_p is that it regresses
the following C++ testcase:

struct z {
  static const int aaa = 1;
};

//const int z::aaa;

int foo(int x)
{
  return x ? z::aaa : x;
}

Here decl_replaceable_p is 'true' for z::aaa.  With the patch the reference to
z::aaa is not folded, but its definition is not emitted either, so a undefined
reference error is produced at link time.  But naturally
varpool_ctor_useable_for_folding_p for z::aaa must stay true in the first place.

In a way z::aaa is "replaceable" in the sense that the compiler is not going
to emit a definition, so if anything references z::aaa in the current
translation unit (if the address is taken), the definition must come from
another TU.  Nevertheless, references to the value can be folded.  I'm
unsure what the correct test would look like.  Any advice?

Thanks.
Alexander

diff --git a/gcc/varpool.c b/gcc/varpool.c
index b98fc1b..addbe6d 100644
--- a/gcc/varpool.c
+++ b/gcc/varpool.c
@@ -329,11 +329,13 @@ varpool_ctor_useable_for_folding_p (varpool_node *node)
   if (!TREE_READONLY (node->decl) && !TREE_READONLY (real_node->decl))
     return false;
 
-  /* Variables declared 'const' without an initializer
-     have zero as the initializer if they may not be
-     overridden at link or run time.  */
-  if (!DECL_INITIAL (real_node->decl)
-      && (DECL_EXTERNAL (node->decl) || decl_replaceable_p (node->decl)))
+  /* Accept variables declared 'const' without an initializer (so they have
+     zero as the initializer), unless they may be overridden at link time.  */
+  if (!DECL_INITIAL (real_node->decl) && DECL_EXTERNAL (node->decl))
+    return false;
+
+  /* Reject initializers that can be overridden at link or run time.  */
+  if (decl_replaceable_p (node->decl))
     return false;
 
   /* Variables declared `const' with an initializer are considered

Re: [PATCH] proposed fix for bug # 61144

[Rich Felker]

On Tue, Jul 22, 2014 at 09:17:12PM +0400, Alexander Monakov wrote:
> On Tue, 22 Jul 2014, Alexander Monakov wrote:
> > I'd like to push this topic forward a bit.  I've bootstrapped and regtested a
> > version of the patch based on the initial proposal to check DECL_WEAK.  The
> > approach with decl_replaceable_p looks not that easy; I'll expand in a
> > followup email.
> 
> The problem with the patch below using decl_replaceable_p is that it regresses
> the following C++ testcase:
> 
> struct z {
>   static const int aaa = 1;
> };
> 
> //const int z::aaa;
> 
> int foo(int x)
> {
>   return x ? z::aaa : x;
> }
> 
> Here decl_replaceable_p is 'true' for z::aaa.  With the patch the reference to
> z::aaa is not folded, but its definition is not emitted either, so a undefined
> reference error is produced at link time.  But naturally
> varpool_ctor_useable_for_folding_p for z::aaa must stay true in the first place.
> 
> In a way z::aaa is "replaceable" in the sense that the compiler is not going
> to emit a definition, so if anything references z::aaa in the current
> translation unit (if the address is taken), the definition must come from
> another TU.  Nevertheless, references to the value can be folded.  I'm
> unsure what the correct test would look like.  Any advice?

Why is this considered "replaceable"? That sounds like a bug to me.

Anyway, in looking for a solution, wouldn't it be helpful to look at
how this was treated in 4.8 and earlier, before bug 61144 was
introduced?

Rich

Re: [PATCH] proposed fix for bug # 61144

[Florian Weimer]

On 07/22/2014 07:17 PM, Alexander Monakov wrote:
> On Tue, 22 Jul 2014, Alexander Monakov wrote:
>> I'd like to push this topic forward a bit.  I've bootstrapped and regtested a
>> version of the patch based on the initial proposal to check DECL_WEAK.  The
>> approach with decl_replaceable_p looks not that easy; I'll expand in a
>> followup email.
>
> The problem with the patch below using decl_replaceable_p is that it regresses
> the following C++ testcase:
>
> struct z {
>    static const int aaa = 1;
> };
>
> //const int z::aaa;
>
> int foo(int x)
> {
>    return x ? z::aaa : x;
> }
>
> Here decl_replaceable_p is 'true' for z::aaa.  With the patch the reference to
> z::aaa is not folded, but its definition is not emitted either, so a undefined
> reference error is produced at link time.

Technically, this is not a bug (for C++03 at least).  But I do think 
folding the constant is an important optimization.

-- 
Florian Weimer / Red Hat Product Security