[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: lib/Sema/AnalysisBasedWarnings.cpp:530
+  // Don't need to mark Objective-C methods or blocks since the undefined
+  // behaviour optimization isn't used for them.
+}
----------------
Quuxplusone wrote:
> This seems like a trap waiting to spring on someone, unless there's a technical reason that methods and blocks cannot possibly use the same optimization paths as regular functions. ("Nobody's gotten around to implementing it yet" is the most obvious nontechnical reason for the current difference.) Either way, I'd expect this patch to include test cases for both methods and blocks, to verify that the behavior you expect is actually the behavior that happens. Basically, it ought to have a regression test targeting the regression that I'm predicting is going to spring on someone as soon as they implement optimizations for methods and blocks.
> 
> Also, one dumb question: what about C++ lambdas? are they FunctionDecls too? test case?
> This seems like a trap waiting to spring on someone, unless there's a technical reason that methods and blocks cannot possibly use the same optimization paths as regular functions.

The optimization path in LLVM is the same. I think the difference lies in clang IRGen: there is no "unreachable" generated for these so the optimizer can't be aggressive. So this patch is not changing anything for Objective-C methods and blocks, and I expect that we *already* have a test that covers this behavior (if not we should add one).


================
Comment at: test/CodeGenCXX/return.cpp:21
+  // CHECK-NOSTRICT-NEXT: load
+  // CHECK-NOSTRICT-NEXT: ret i32
+  // CHECK-NOSTRICT-NEXT: }
----------------
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. 


================
Comment at: test/CodeGenCXX/return.cpp:35
+// CHECK-NOSTRICT:     @_Z21alwaysOptimizedReturn4Enum
+// CHECK-NOSTRICT-OPT: @_Z21alwaysOptimizedReturn4Enum
+int alwaysOptimizedReturn(Enum e) {
----------------
This should be `-LABEL` check lines. And instead of repeating 4 times the same check, you could add a common prefix.


================
Comment at: test/CodeGenCXX/return.cpp:47
+  // CHECK-NOSTRICT-OPT-NEXT: zext
+  // CHECK-NOSTRICT-OPT-NEXT: ret i32
+}
----------------
Document what's going on in the tests please.



Repository:
  rL LLVM

https://reviews.llvm.org/D27163