Page MenuHomePhabricator

[InstCombine] Preserve !nosanitize for newly created instructions.
Needs ReviewPublic

Authored by Enna1 on May 23 2022, 9:11 PM.

Details

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.

Diff Detail

Event Timeline

Enna1 created this revision.May 23 2022, 9:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2022, 9:11 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
Enna1 published this revision for review.May 23 2022, 9:57 PM
Enna1 edited the summary of this revision. (Show Details)
Enna1 added reviewers: fhahn, nikic, vitalybuka, dvyukov.
Enna1 added a subscriber: MTC.
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2022, 9:58 PM

We need a test for that

nikic added a comment.May 24 2022, 2:52 AM

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.

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

Enna1 updated this revision to Diff 431879.May 24 2022, 9:41 PM
Enna1 edited the summary of this revision. (Show Details)

Update patch

Enna1 added a comment.May 25 2022, 5:22 AM

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

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 added a comment.May 25 2022, 2:02 PM

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 added a comment.May 25 2022, 6:54 PM

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 added a comment.EditedMay 30 2022, 2:24 AM

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