Patch for inline abort code generation

[danielaustin]

Summary

Included is a patch to CGExpr.cpp to allow for trap calls to be generated inline as opposed to one per function. This has
been found to aid in debugging SIGABRTs triggered by integer overflows in variables that do not influence a memory access in
AOSP.

To check the size impact of inlining calls to abort, 57 binaries were selected from AOSP that have integer sanitization turned on.
The system was compiled with default options for a 32-bit build (shamu) and a 64-bit build (angler). The resulting build has been tested
on a Nexus 6 (shamu) device with no stability issues.

For the 32-bit build, the average file increase was 3.43%. The greatest size increase was 33.88% (libstagefright_m4vh263enc.a, 934630 to 1251270),
and one file actually became slightly smaller libstagefright_timedtext.a, from 750214 to 749886.

For the 64-bit build, the average file increase was 5.35%. The greatest size increase was 41.86% (libstagefright_m4vh263enc.a, 1288622 to 1828030).
Four files were slightly smaller:
init: 1269536 to 1257248
libfs_mgr.a: 262288 to 258776
libinit.a: 4442000 to 4326804
libstagefright_timedtext.a: 892450 to 890250

As for generated code, a function known to have multiple integer overflows, search_ixiy in libstagefright_amrwbenc.so was analyzed
in more depth. The function has 6 calls to abort, all of which are generated inline with the patched compiler. The original 32 bit
version is 536 bytes in size, while the inline abort version is 656. The original 64 bit version is 544, while the inline abort version is 648.

I am happy to provide more detail or perform additional analysis if it helps. From what I've gathered so far, this may not be the best option to enable
by default, but as a debug option, it would be very valuable to anyone developing with the sanitizer.

[danielaustin]

Forgot to upload the version with the EmptyAsm call; does not properly inline trap calls otherwise.

[rsmith]

I would expect that people who are using this for hardening would be upset about a 5% binary size increase. I'm OK with having this behind a flag, though.

[samsonov]

I agree with Richard here.

[danielaustin]

Makes sense, will add that as well.

[danielaustin]

Added a flag controlling whether or not the inlining occurs. Tested with ARM 32 bit (shamu) with AOSP. The patched compiler resulted in correct code generation and no stability issues. The flag is probably not ideally named, but treating it as a sanitizer did allow for easy integration into the Android build system.

[samsonov]

Sorry for the late response. Ugh, flag naming is hard, and it's far too complicated for sanitizers already.

However, I'm opposed to passing this down as -fsanitize= option =/. So far we're trying to make values of -fsanitize= correspond *only* to different kinds of checks, not configuration modes. For instance, we've pushed back against somewhat similar feature request for CFI: original plan for enabling "debug mode" in CFI was to pass "-fsanitize=cfi-diag", but we instead changed it to be "-fsanitize=cfi -fno-sanitize-trap=cfi".

And we have three possible configurations: recoverable checks, fatal/unrecoverable checks, trapping checks. You need, essentially "trapping-but-distinguishable checks". I'm afraid adding fourth configuration will make things incomprehensible. -fsanitize-merge-traps (true by default)?

[samsonov]

That is, I presume it's highly unlikely user will need to have fine-grained setup for deciding which UBSan checks should be merged, and which should not. So, probably having a single -fsanitize-merge-traps / -fno-sanitize-merge-traps option would be enough. I'd like to hear other opinions, though.

[eugenis]

Better -fsanitize-merge-checks, and it should apply to non-trap checks as well (i.e. SIGILL address should uniquely correspond to the failure source location).

[danielaustin]

I'm not partial to what its called, or where it is. I'm completely ok with
-fsanitize-merge-checks

[samsonov]

That is, I presume it's highly unlikely user will need to have fine-grained setup for deciding which UBSan checks should be merged, and which should not. So, probably having a single -fsanitize-merge-traps / -fno-sanitize-merge-traps option would be enough. I'd like to hear other opinions, though.

In D15208#305199, @eugenis wrote:

Better -fsanitize-merge-checks, and it should apply to non-trap checks as well (i.e. SIGILL address should uniquely correspond to the failure source location).

Just to make sure I understand it, you suggest to eventually pass it down to ASan/MSan instrumentation passes?

[eugenis]

I misunderstood the meaning of -fsanitize-trap, and now I prefer -fsanitize-merge-traps for the flag name.

[danielaustin]

I don't think super-fine grained control would be required, a per-binary
setting should be sufficient for debugging purposes.

If everyone is ok with it, I'll get a diff up later tonight switching the
flag to fsanitize-merge-traps/fnosanitize-merge-traps

[danielaustin]

Patch that removes the debug-mode sanitizer setting and makes the -fsanitize-merge-traps/-fno-sanitize-merge-traps flags, with the default setting being to merge traps. Flags tested within the Android build system, with presence and absence of flags both working as expected.

[samsonov]

Please consider adding a test case.

include/clang/Basic/LangOptions.h

95 ↗

(On Diff #42328)

Why is it a language, not codegen option?

97 ↗

(On Diff #42328)

MergeTraps

include/clang/Driver/Options.td

614 ↗

(On Diff #42328)

These should probably be CC1Option as well.

lib/CodeGen/CGExpr.cpp

2543

Note that this will also affect -ftrapv, which might be unexpected, as we mention "sanitize" in the flag name.

2549

Looks like most of this block (everything except for the empty asm statement) is duplicated below.

lib/Frontend/CompilerInvocation.cpp

1572 ↗

(On Diff #42328)

Args.hasFlag(OPT_fsanitize_merge_traps, OPT_fno_sanitize_merge_traps, true);

[danielaustin]

Test makes sense, will add it with the revisions.

include/clang/Basic/LangOptions.h

95 ↗

(On Diff #42328)

Wasn't aware, will relocate it there, as it makes more sense

include/clang/Driver/Options.td

614 ↗

(On Diff #42328)

Agree

lib/CodeGen/CGExpr.cpp

2543

Would it be better in your opinion to remove sanitize from the name, or check for the presence of the integer sanitizers here?

2549

Yea, I can simplify that.

[samsonov]

lib/CodeGen/CGExpr.cpp

2543

Probably former - we already have -ftrap-function which changes the behavior of both sanitizer-generated and regular traps - this flag is no different.

[danielaustin]

Test case added which looks for the existence of multiple trap basic blocks in generated LLVM-IR with optimization turned on. Flag renamed (-fmerge-traps/-fno-merge-traps) and placed in a more sensible location (CodeGenOptions.def). Trap inlining logic changed to work with most recent version of LLVM (as opposed to the one packaged with AOSP). Without turning optimization off in the function containing the trap, there were cases that the abort calls were merged to the end of the function. Turning off inlining and optimization for a function which is not to merge traps appears to be working, but further analysis is in process.

This version results in slightly bigger code (~10% per function) than the original version, but has the benefit of not breaking basic blocks.

[samsonov]

Sorry for delay.

lib/CodeGen/CGExpr.cpp

2543

This condition is somewhat hard to parse. Consider inverting it:

// Collapse all trap calls to one per function if we're optimizing, and ....
if (TrapBB && CGM.getCodeGenOpts().OptimizationLevel && CGM.getCodeGenOpts().MergeTraps) {
  Builder.CreateCondBr(Checked, Cont, TrapBB);
} else {
   //...
}

2557

Nit: AddressSanitizer, or ASan.

2558

What's wrong with marking trap function as noreturn anyway? unreachable instruction is, well, supposed to be unreachable.

2560

This can also be hoisted from branches.

lib/CodeGen/CGExpr.cpp

 void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked) {
   llvm::BasicBlock *Cont = createBasicBlock("cont");
 
+  // Collapsing all calls to trap down to one per function makes debugging
+  // these issues much more difficult. Eliminating this optimization for now,
+  // with the thought that control by either a debug flag or tying it to an 
+  // optimization level/sanitizer may be a decent path for the future.
+  // RE: Bug: 25682
   // If we're optimizing, collapse all calls to trap down to just one per
   // function to save on code size.
-  if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
-    TrapBB = createBasicBlock("trap");
-    Builder.CreateCondBr(Checked, Cont, TrapBB);
-    EmitBlock(TrapBB);
-    llvm::CallInst *TrapCall = EmitTrapCall(llvm::Intrinsic::trap);
-    TrapCall->setDoesNotReturn();
-    TrapCall->setDoesNotThrow();
-    Builder.CreateUnreachable();
-  } else {
-    Builder.CreateCondBr(Checked, Cont, TrapBB);
-  }
+  //  if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
+  //} else {
+  //Builder.CreateCondBr(Checked, Cont, TrapBB);
+  //}
 
   EmitBlock(Cont);
 }