Re: [PATCH] proposed fix for bug # 61144

[Rich Felker <dalias at libc dot org>]

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