[Bug c++/70018] New: Possible issue around IPO and C++ inline functions

[sanjoy at playingwithpointers dot com]

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

Bug ID: 70018
   Summary: Possible issue around IPO and C++ inline functions
   Product: gcc
   Version: unknown
Status: UNCONFIRMED
  Severity: normal
  Priority: P3
 Component: c++
  Assignee: unassigned at gcc dot gnu.org
  Reporter: sanjoy at playingwithpointers dot com
  Target Milestone: ---

I don't grok gcc internals so I cannot write a terribly well-informed
bug report, but GCC 5.3.0 seems to miscompile
https://github.com/sanjoy/comdat-ipo

This was discussed on llvm-dev:
http://lists.llvm.org/pipermail/llvm-dev/2016-February/095833.html and
the thread contains a description of the underlying cause for
LLVM/Clang.

The TL;DR is that for C++ inline functions (and other functions with
similar linkage rules), you can override a more-refined function
implementation with a less-refined one at link time, and that can
retroactively invalidate earlier transforms, where "refined" in this
case means "undefined in fewer situations".

E.g. if we have (this is very similar to the reproducer above minus
some mechanical details):

inline void foo(int* ptr) {
  100 / ptr[0];
}

void bar(int* ptr) {
  *ptr = 40;
  foo(ptr):
  *ptr = 50;
}

=>

inline void foo(int* ptr) {
  // 100 / ptr[0]; removed, dead code
}

void bar(int* ptr) {
  *ptr = 40;
  foo(ptr):
  *ptr = 50;
}

=>

inline void foo(int* ptr) {
  // 100 / ptr[0]; removed, dead code
}

void bar(int* ptr) {
  // *ptr = 40; dead store, since foo does not read memory
  foo(ptr):
  *ptr = 50;
}

we've miscompiled if *ptr == 0 on entry to bar, and foo is replaced
with the original definition at link time.

[Bug c++/70018] Possible issue around IPO and C++ inline functions

[sanjoy at playingwithpointers dot com]

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

--- Comment #3 from Sanjoy Das  ---
(In reply to Andrew Pinski from comment #1)
> In C++ code, the one definition rule says that all TU that contains an
> inline function, they need to have the same definition.  If they have
> different definition, then the code is undefined at runtime and no
> diagnostic is required.

Hi Andrew,

I believe that is an incorrect assessment of the test case:
https://github.com/sanjoy/comdat-ipo

At the C++ source level there is only one definition of maybe_divide
that is in header.hpp included by both source-a.cpp and source-b.cpp.
If this violates ODR then every C++ program with inline functions in
the header violates ODR. :)

[Bug c++/70018] Possible issue around IPO and C++ inline functions

[sanjoy at playingwithpointers dot com]

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

--- Comment #4 from Sanjoy Das  ---
(In reply to Andrew Pinski from comment #2)

> So in summary what you are seeing is two effects going into effect here:
> undefined behavior of division by 0 and ODR.

There is no division by zero (or any other UB that I'm aware of) in the
original program.

[Bug c++/70018] Possible issue around IPO and C++ inline functions

[sanjoy at playingwithpointers dot com]

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

--- Comment #6 from Sanjoy Das  ---
(In reply to Andrew Pinski from comment #5)
> Oh I see pure/const behavior.
> 
> The problem is more complex, in that in one TU, the comdat function is
> figured out to be pure/const so we remove the store before the function
> call.  While in the other we don't.  Now we got a comdat function which is
> pure in one TU while not in the other case.  We use the non pure version of
> the comdat function.
> 
> I thought this was fixed in 6.

Ah, okay.  I'm testing with 5.3.  If this is fixed in gcc 6, please feel free
to close.