[InstCombine] Preserve !nosanitize for newly created instructions.

[Enna1]

Summary

If the source instruction has !nosanitize metadata, all instructions created during combining should also have it.

The !nosanitize metadata indicates that LLVM should not insert any sanitizer instrumentation.

This https://github.com/google/sanitizers/issues/1508 false positive is caused:

1. InstCombine do not preserve !nosanitize for newly created instructions if source instruction has !nosanitize metadata and
2. ASan instruments for pointer comparison and subtraction instruction even if it has !nosanitize metadata

This patch and D126269 together will fix the false positive.

[vitalybuka]

We need a test for that

[nikic]

This change would have a very large compile-time impact: http://llvm-compile-time-tracker.com/compare.php?from=a7c079aaa227d55ad9cb6b916500d99b4fdf26d2&to=0d55a5d6fc90d772d43b518f5cefa41262ccce2d&stat=instructions

I'd suggest to first land a patch that adds MD_nosanitize, and then try this again. I suspect it will still have significant negative impact, but probably not quite as large.

[vitalybuka]

Please also link patches related to #1508 into review "Stack" (see "Edit Related Revisions" on the menu)

[Enna1]

Update patch

[Enna1]

@nikic can you help try llvm-compile-time-tracker for this version patch again, thanks!

[efriedma]

It might make sense to take a step back, and look again at how ASan works here.

There's a comment in AddressSanitizer.cpp: "This is a rough heuristic; it may cause both false positives and false negatives. The proper implementation requires cooperation with the frontend." Instead of trying to preserve metadata on random instructions, can we fix pointer subtraction/comparison under asan to use some dedicated intrinsic that reflects the necessary semantics?

(See also discussion on https://discourse.llvm.org/t/rfc-safe-optimizations-for-sanitizers/62729 .)

[nikic]

Compile-time for the new version: https://llvm-compile-time-tracker.com/compare.php?from=3e7285426d979eb1719b88e76b3a63ea78dafb60&to=b8e2e12990da9c34c8bc7a7d52b02e825fbd3877&stat=instructions This is much better than before, though still fairly expensive. I'd suggest landing the MD_nosanitize patch regardless of whether we make this change, as it's clearly beneficial to have a known ID for this.

I haven't looked into the general problem being solved here, but if this is a matter of correctness, then @efriedma's suggestion is probably the right one. Metadata preservation in LLVM is best-effort and dropping metadata should never result in a miscompile (which a sanitizer false positive arguably is).

[Enna1]

Thanks @efriedma for pointing out this discussion thread https://discourse.llvm.org/t/rfc-safe-optimizations-for-sanitizers/62729.
I'll land the MD_nosanitize patch(D126294) first as @nikic suggestion.
And I will try if we can fix ASan's pointer subtraction/comparison check with cooperation of frontend.

[Enna1]

I came up a workaround, but it may not elegant enough i think.
In AddressSanitizer.cpp isInterestingPointerComparison(), we add a check if the two following conditions satisfied:

• the ICmpInst is used as a branchinst predicate
• the branchinst has a successor containing callback inserted by UBSan(e.g. __ubsan_handle_pointer_overflow)

If so, ASan will not take this ICmpInst as an interesting pointer compairson, and will not have a false positive on this.

define dso_local i32 @main() local_unnamed_addr #0 !dbg !9 {
  %1 = alloca [10 x i8], align 1
  call void @llvm.dbg.declare(metadata [10 x i8]* %1, metadata !14, metadata !DIExpression()), !dbg !19
  %2 = ptrtoint [10 x i8]* %1 to i64, !dbg !20, !nosanitize !13
  %.not = icmp ugt [10 x i8]* %1, inttoptr (i64 -5 to [10 x i8]*), !dbg !20
  br i1 %.not, label %3, label %5, !dbg !20, !prof !21, !nosanitize !13

3:                                                ; preds = %0
  %4 = add i64 %2, 4, !dbg !20, !nosanitize !13
  call void @__ubsan_handle_pointer_overflow(i8* bitcast ({ { [7 x i8]*, i32, i32 } }* @0 to i8*), i64 %2, i64 %4) #3, !dbg !20, !nosanitize !13
  br label %5, !dbg !20, !nosanitize !13

[vitalybuka]

I suspect it's abandoned?

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

if (OptBB) {
           if (Instruction *OpI = dyn_cast<Instruction>(U.get()))
             Worklist.push(OpI);
       }
     }
 
     // Now that we have an instruction, try combining it to simplify it.
     Builder.SetInsertPoint(I);
     Builder.CollectMetadataToCopy(
-        I, {LLVMContext::MD_dbg, LLVMContext::MD_annotation});
+        I, {LLVMContext::MD_dbg, LLVMContext::MD_annotation,
+            I->getModule()->getMDKindID("nosanitize")});
 
 #ifndef NDEBUG
     std::string OrigI;
 #endif
     LLVM_DEBUG(raw_string_ostream SS(OrigI); I->print(SS); OrigI = SS.str(););
     LLVM_DEBUG(dbgs() << "IC: Visiting: " << OrigI << '\n');
 
     if (Instruction *Result = visit(*I)) {
       ++NumCombined;
       // Should we replace the old instruction with a new one?
       if (Result != I) {
         LLVM_DEBUG(dbgs() << "IC: Old = " << *I << '\n'
                           << "    New = " << *Result << '\n');
 
         Result->copyMetadata(*I,
-                             {LLVMContext::MD_dbg, LLVMContext::MD_annotation});
+                             {LLVMContext::MD_dbg, LLVMContext::MD_annotation,
+                              I->getModule()->getMDKindID("nosanitize")});
         // Everything uses the new instruction now.
         I->replaceAllUsesWith(Result);
 
         // Move the name to the new instruction first.
         Result->takeName(I);
 
         // Insert the new instruction into the basic block...
         BasicBlock *InstParent = I->getParent();