[PATCH] D53459: Ensure sanitizer check function calls have a !dbg location

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.


================
Comment at: lib/CodeGen/CGExpr.cpp:2871
+  auto *DI = CGF.getDebugInfo();
+  SourceLocation Loc = DI ? DI->getLocation() : SourceLocation();
+  auto DL = ApplyDebugLocation::CreateDefaultArtificial(CGF, Loc);
----------------
aprantl wrote:
> vsk wrote:
> > Why shouldn't this always be line 0? A call to a check handler is always auto-generated.
> I was thinking that it might be nice to have the source location of the expression with the UB in the backtrace, too — not just in the error message. (Which is the current behavior by accident, since no location means carry over the previous instruction's location). But I'm fine with just setting line 0, too.
I see, that seems reasonable.


================
Comment at: test/CodeGenCXX/ubsan-check-debuglocs.cpp:2
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=limited \
+// RUN:   -fsanitize=null,object-size,return -fsanitize-recover=null \
+// RUN:   %s -o - | FileCheck %s
----------------
aprantl wrote:
> vsk wrote:
> > Are all three sanitizers needed here to reproduce the bug? Seems like a simpler test would be:
> > 
> > ```
> > // RUN: ... -fsanitize=null ...
> > 
> > int deref(int *p) { return *p; }
> > // CHECK-LABEL: @deref
> > // CHECK: __ubsan_handle_type_mismatch_v1{{.*}} !dbg [[ubsan_handler_loc:![0-9]+]]
> > // CHECK: [[ubsan_handler_loc]] = !DILocation(line: 0
> > ```
> Probably not for the testcase; only to trigger the subsequent crash.
I see -- we probably don't need the full crash repro for this change. At any rate I don't think we support ubsan-itizing the ubsan runtime..


https://reviews.llvm.org/D53459