[Bug middle-end/95558] New: Invalid IPA optimizations based on weak definition

[Bug middle-end/95558] New: Invalid IPA optimizations based on weak definition

[bugdal at aerifal dot cx]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

            Bug ID: 95558
           Summary: Invalid IPA optimizations based on weak definition
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: bugdal at aerifal dot cx
  Target Milestone: ---

Created attachment 48689
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48689&action=edit
test case

Here is a case that came up in WIP code on musl libc, where I wanted to provide
a weak dummy definition for functionality that would optionally be replaced by
a strong definition elsewhere at ld time. I've been looking for some plausible
explanation aside from an IPA bug, like interaction with UB, but I can't find
any.

In the near-minimal test case here, the function reclaim() still has all of the
logic it should, but reclaim_gaps gets optimized down to a nop.

What seems to be happening is that the dummy weak definition does not leak into
its direct caller via IPA optimizations, but does leak to the caller's caller.

[Bug ipa/95558] Invalid IPA optimizations based on weak definition

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amonakov at gcc dot gnu.org,
                   |                            |marxin at gcc dot gnu.org
          Component|middle-end                  |ipa
           Keywords|                            |wrong-code

--- Comment #1 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
All functions are incorrectly discovered to be pure, and then the loop that
only makes calls to non-weak pure functions is eliminated.

Minimal testcase for the root issue, wrong warning with -O2
-Wsuggest-attribute=pure:

static void dummy(){}

void weak() __attribute__((weak,alias("dummy")));

int foo()
{
    weak();
    return 0;
}

[Bug ipa/95558] Invalid IPA optimizations based on weak definition

[bugdal at aerifal dot cx]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

--- Comment #2 from Rich Felker <bugdal at aerifal dot cx> ---
Wow. It's interesting that we've never seen this lead to incorrect codegen
before, though. All weak dummies should be affected, but only in some cases
does the pure get used to optimize out the external call.

This suggests there's a major missed optimization around pure functions too, in
addition to the wrong application of pure (transfering it from the weak
definition to the external declaration) that's the bug.

[Bug ipa/95558] Invalid IPA optimizations based on weak definition

[bugdal at aerifal dot cx]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

--- Comment #3 from Rich Felker <bugdal at aerifal dot cx> ---
In addition to a fix, this is going to need a workaround as well. Do you have
ideas for a clean one? A dummy asm in the dummy function to kill pureness is
certainly a big hammer that would work, but it precludes LTO optimization if
the weak definition doesn't actually get replaced, so I don't like that.

One idea I think would work, but not sure: make an external __weak_dummy_tail
function that all the weak dummies tail call to. This should only take a few
bytes more than just returning, and precludes pureness analysis in the TU it's
in, while still allowing DCE at LTO time when the definition of
__weak_dummy_tail becomes available.

Is my reasoning correct here?

[Bug ipa/95558] Invalid IPA optimizations based on weak definition

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at gcc dot gnu.org
            Version|unknown                     |10.1.0
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2020-06-09

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
Confirmed.

[Bug ipa/95558] [9/10/11/12 Regression] Invalid IPA optimizations based on weak definition

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |4.7.1
      Known to work|                            |4.5.3, 4.6.4
   Target Milestone|---                         |9.5
            Summary|Invalid IPA optimizations   |[9/10/11/12 Regression]
                   |based on weak definition    |Invalid IPA optimizations
                   |                            |based on weak definition

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Note you need -fno-inline to produce the issue these days.
Plus it is a regression from GCC 4.6.

[Bug ipa/95558] [9/10/11/12 Regression] Invalid IPA optimizations based on weak definition

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
Honza?  What's your stance here?  Do weak aliases fall under some implicit ODR
here?

[Bug ipa/95558] [9/10/11/12 Regression] Invalid IPA optimizations based on weak definition

[bugdal at aerifal dot cx]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

--- Comment #7 from Rich Felker <bugdal at aerifal dot cx> ---
> Do weak aliases fall under some implicit ODR here?

The whole definition of "weak" is that it entitles you to make a definition
that will be exempt from ODR, where a non-weak definition, if any, replaces it.

[Bug ipa/95558] [9/10/11/12 Regression] Invalid IPA optimizations based on weak definition

[hubicka at kam dot mff.cuni.cz]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

--- Comment #8 from hubicka at kam dot mff.cuni.cz ---
> > Do weak aliases fall under some implicit ODR here?
> 
> The whole definition of "weak" is that it entitles you to make a definition
> that will be exempt from ODR, where a non-weak definition, if any, replaces it.

I fixed recently bug in pure-const discovery which made us to add
attributes to weaks while it should not, so perhaps this bug is gone.
For non-inline functions we should handle those as interposable.
Concerning warning, I we make difference between may and must warnings.
I think this is still useful do be diagnosed for user to consider
(perhaps there is a reason why all overwritters has to be weak), so I
think we should porduce may form of warning.

I will take a look.

[Bug ipa/95558] [9/10/11/12 Regression] Invalid IPA optimizations based on weak definition

[bugdal at aerifal dot cx]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

--- Comment #9 from Rich Felker <bugdal at aerifal dot cx> ---
Can you provide a link to the commit that might have fixed it? I imagine it's
simple enough to backport, in which case I'd like to do so.

[Bug ipa/95558] [9/10/11/12 Regression] Invalid IPA optimizations based on weak definition

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

--- Comment #10 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
As comment #5 mentioned, it is still broken, you just need -fno-inline in
addition to -O2 for the original testcase. Andrew's remark is quite useful for
situations like this, you know :)

[Bug ipa/95558] [9/10/11/12 Regression] Invalid IPA optimizations based on weak definition

[bugdal at aerifal dot cx]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

--- Comment #11 from Rich Felker <bugdal at aerifal dot cx> ---
Are you sure? If pure/const discovery is no longer applied to weak definitions,
it shouldn't be able to propagate to a non-inlined caller. Of course the fix
may be incomplete or not working, which I guess we could tell from whether it
happened prior to or after comment 5. :)

[Bug ipa/95558] [9/10/11/12 Regression] Invalid IPA optimizations based on weak definition

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

--- Comment #12 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Rich Felker from comment #11)
> Are you sure? If pure/const discovery is no longer applied to weak
> definitions, it shouldn't be able to propagate to a non-inlined caller. Of
> course the fix may be incomplete or not working, which I guess we could tell
> from whether it happened prior to or after comment 5. :)

There still looks like there is a bug as shown by taking the original testcase
in comment #0 and using -O2 -fno-inline, you will get:

reclaim_gaps:
        ret
...
foo:
        ret

Which is still wrong as reclaim should not be considered as pure.

>From *.pure-const:

Starting cycle
  Visiting donate_dummy/0 state:const looping 0
Result const looping 0
Function found not to call free: donate_dummy/0
Starting cycle
  Visiting reclaim/2 state:pure looping 0
    Call to __malloc_donate/1 const
Result pure looping 0
Function found to be pure: reclaim/2
Declaration updated to be pure: reclaim/2
Starting cycle
  Visiting reclaim_gaps/3 state:pure looping 0
    Call to reclaim/2 state:pure looping:0
    Call to reclaim/2 state:pure looping:0
Result pure looping 0
Function found to be pure: reclaim_gaps/3
Declaration updated to be pure: reclaim_gaps/3
Starting cycle
  Visiting foo/4 state:const looping 0
    Call to reclaim_gaps/3 state:pure looping:0
Result pure looping 0
Function found to be pure: foo/4
Declaration updated to be pure: foo/4

reclaim, reclaim_gaps and foo are all found as pure. without -fno-inline, we
get some early inlining which causes the difference there.

[Bug ipa/95558] [9/10/11/12 Regression] Invalid IPA optimizations based on weak definition

[hubicka at kam dot mff.cuni.cz]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

--- Comment #13 from hubicka at kam dot mff.cuni.cz ---
> Result pure looping 0
> Function found to be pure: foo/4
This is good - we are supposed to find it to be pure and walk all
aliases and update noninterposable ones
> Declaration updated to be pure: foo/4
I have to take look why this happens though.

[Bug ipa/95558] [9/10/11/12 Regression] Invalid IPA optimizations based on weak definition

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

[Bug ipa/95558] [10/11/12/13 Regression] Invalid IPA optimizations based on weak definition

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|9.5                         |10.4

--- Comment #14 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 9 branch is being closed

[Bug ipa/95558] [10/11/12/13 Regression] Invalid IPA optimizations based on weak definition

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.4                        |10.5

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 10.4 is being released, retargeting bugs to GCC 10.5.

[Bug ipa/95558] [10/11/12/13 Regression] Invalid IPA optimizations based on weak definition

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2020-06-09 00:00:00         |2023-1-19
           Assignee|unassigned at gcc dot gnu.org      |hubicka at gcc dot gnu.org

--- Comment #16 from Richard Biener <rguenth at gcc dot gnu.org> ---
Re-confirmed.  The bad attribute suggestion is also still there for the
testcase in comment#1

[Bug ipa/95558] [11/12/13/14 Regression] Invalid IPA optimizations based on weak definition

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95558

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.5                        |11.5

--- Comment #17 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10 branch is being closed.