[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.


================
Comment at: test/CodeGenCXX/return.cpp:21
+  // CHECK-NOSTRICT-NEXT: load
+  // CHECK-NOSTRICT-NEXT: ret i32
+  // CHECK-NOSTRICT-NEXT: }
----------------
rjmccall wrote:
> mehdi_amini wrote:
> > Quuxplusone wrote:
> > > Can you explain why a load from an uninitialized stack location would be *better* than a trap and/or `unreachable`? IIUC this is basically what Mehdi is asking: i.e., can you explain the rationale for this patch, because I don't get it either. It *seems* like a strict regression in code quality.
> > There is a difference. LLVM will optimize:
> > 
> > ```
> > define i32 @foo() {
> >   %1 = alloca i32, align 4
> >   %2 = load i32, i32* %1, align 4
> >   ret i32 %2
> > }
> > ```
> > 
> > to:
> > 
> > ```
> > define i32 @foo() {
> >   ret i32 undef
> > }
> > ```
> > 
> > Which means "return an undefined value" (basically any valide bit-pattern for an i32). This is not undefined behavior if the caller does not use the value with a side-effect.
> > 
> > However with:
> > 
> > ```
> > define i32 @foo() {
> >   unreachable
> > }
> > ```
> > 
> > Just calling `foo()` is undefined behavior, even if the returned value isn't used.
> > 
> > So what this patch does is actually making the compiled-code `safer` by inhibiting some optimizations based on this UB. 
> We've been disabling this optimization completely in Apple compilers for years, so allowing it to ever kick in is actually an improvement.  I'll try to elaborate on our reasoning for this change, which *is* actually somewhat Apple-specific.
> 
> Falling off the end of a non-void function is only undefined behavior in C++, not in C.  It is fairly common practice to compile C code as C++, and while there's a large number of potential language incompatibilities there, they tend to be rare or trivial to fix in practice.  At Apple, we have a specific need to compile C code as C++ because of the C++-based IOKit API: while drivers are overwhelmingly written as C code, at Apple they have to be compiled as C++ in order to talk to the kernel.  Moreover, often Apple did not write the drivers in question, and maintaining a large set of patches in order to eliminate undefined behavior that wasn't actually UB in the originally-intended build configuration is not really seen as acceptable.
> 
> It makes sense to me to expose this as a flag akin to -fno-strict-aliasing in order to support C-in-C++ code bases like Apple's drivers.  It is possible that we don't actually have to change the default for the flag on Apple platforms and can instead pursue more targeted build changes.
I totally understand why such flag is desirable, it is just not a clear cut to make it the default on one platform only (and having this flag available upstream does not prevent the Xcode clang from enabling this by default though, if fixing the build settings isn't possible/desirable). 


Repository:
  rL LLVM

https://reviews.llvm.org/D27163