[ASan] Use debuginfo for symbolization.

[hctim]

Summary

Hint: Looking here because your manual invocation of something in
'check-asan' broke? You need a new symbolizer (after D123538).

An upcoming patch will remove the internal metadata for global
variables. With D123534 and D123538, clang now emits DWARF debug info
for constant strings (the only global variable type it was missing), and
llvm-symbolizer is now able to symbolize all global variable addresses
(where previously it wouldn't give you the file:line information).

Move ASan's runtime over from the internal metadata to DWARF.

[eugenis]

compiler-rt/lib/asan/asan_globals.cpp

93

remove the "(from debuginfo)" part? It's a waste of bytes :)

compiler-rt/lib/asan/asan_interface_internal.h

56–58

Do we have to do this every time we change a struct definition now? Sounds like a windows problem.

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp

377

How does this fail with the old symbolizer? Is it possible to detect it and be nice about it?

[hctim]

compiler-rt/lib/asan/asan_interface_internal.h

56–58

Yeah, definitely does. Need to sit down with @rnk at some point and figure out how we might best work around this (maybe for COFF we emit a single huge global that's actually an array, or we disable incremental linkage).

FYI - there's a static assertion for this behaviour in compiler-rt/lib/asan/asan_globals.cpp:375 (or grep for "MSVC incremental linker").

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp

377

It just sets them to empty strings. I've also noticed an outdated function comment above, have updated that, and added an explicit comment about why the behaviour succeeds here.

The *-NO-G tests in compiler-rt/test/asan/TestCases/global-location.cpp exercise this behaviour.

[hctim]

(update)

[eugenis]

LGTM

[hctim]

Split global-location.cpp to only run the tests with debuginfo against non-windows, make the linker invoke strip in order to avoid problems with the android wrapper, and remove a not-possible line/column number check from windows (this is no longer possible).

Hopefully the presubmit bot kicks the windows and I can see there.

[hctim]

Looks green on Windows: https://lab.llvm.org/buildbot/#/builders/127/builds/31176 (which was the one I was more concerned about as I couldn't test it).

Waiting on the Android bot, but I tested locally without issue: https://lab.llvm.org/buildbot/#/builders/77/builds/18674 (bot was already red, this should be the first build that actually tests the change).

[thakis]

I'm seeing some warnings after this:

[1/6] CXX stage2_unix/obj/compiler-rt/lib/asan/asan_shared_library.asan_globals.o
../../compiler-rt/lib/asan/asan_globals.cpp:93:52: warning: format specifies type 'int' but the argument has type '__sanitizer::uptr' (aka 'unsigned long') [-Wformat]
    Report("  location: name=%s, %d\n", info.file, info.line);
                                 ~~                ^~~~~~~~~
                                 %lu
../../compiler-rt/lib/asan/asan_globals.cpp:304:37: warning: format specifies type 'int' but the argument has type '__sanitizer::uptr' (aka 'unsigned long') [-Wformat]
    str->append("%s:%d", info.file, info.line);
                    ~~              ^~~~~~~~~
                    %lu

Should we maybe just change DataInfo::line to int, like it is in AddressInfo?

Alternatively, see 44f0e14537801743ffc2c0d40e35b8335d0c1699 for prior art, but printing line numbers as pointers seems a bit weird.

(Or we could just add the explicit cast to int, but 4 billion line numbers should be enough for anyone :) )

But anything that makes the warning go away is fine with me.

[hctim]

In D127552#3645436, @thakis wrote:

I'm seeing some warnings after this:

[1/6] CXX stage2_unix/obj/compiler-rt/lib/asan/asan_shared_library.asan_globals.o
../../compiler-rt/lib/asan/asan_globals.cpp:93:52: warning: format specifies type 'int' but the argument has type '__sanitizer::uptr' (aka 'unsigned long') [-Wformat]
    Report("  location: name=%s, %d\n", info.file, info.line);
                                 ~~                ^~~~~~~~~
                                 %lu
../../compiler-rt/lib/asan/asan_globals.cpp:304:37: warning: format specifies type 'int' but the argument has type '__sanitizer::uptr' (aka 'unsigned long') [-Wformat]
    str->append("%s:%d", info.file, info.line);
                    ~~              ^~~~~~~~~
                    %lu

Should we maybe just change DataInfo::line to int, like it is in AddressInfo?

Alternatively, see 44f0e14537801743ffc2c0d40e35b8335d0c1699 for prior art, but printing line numbers as pointers seems a bit weird.

(Or we could just add the explicit cast to int, but 4 billion line numbers should be enough for anyone :) )

But anything that makes the warning go away is fine with me.

Casted it in 81c48436bbd29736f77a111fc207e28854939907

[thakis]

Thanks!

[aeubanks]

Hint: Looking here because your manual invocation of something in
'check-asan' broke? You need a new symbolizer (after D123538).

with the runtimes build, doing a ninja check-runtimes without building everything beforehand fails because llvm-symbolizer isn't built. we should add llvm-symbolizer as test dependency, although I'm not familiar enough with the compiler-rt testing CMake machinery to know where that would go

compiler-rt/lib/asan/asan_globals.cpp

 }
 
 static void ReportGlobal(const Global &g, const char *prefix) {
   Report(
       "%s Global[%p]: beg=%p size=%zu/%zu name=%s module=%s dyn_init=%zu "
       "odr_indicator=%p\n",
       prefix, (void *)&g, (void *)g.beg, g.size, g.size_with_redzone, g.name,
       g.module_name, g.has_dynamic_init, (void *)g.odr_indicator);
-  if (g.location) {
-    Report("  location (%p): name=%s[%p], %d %d\n", (void *)g.location,
-           g.location->filename, (void *)g.location->filename,
-           g.location->line_no, g.location->column_no);
+
+  DataInfo info;
+  Symbolizer::GetOrInit()->SymbolizeData(g.beg, &info);
+  if (info.line != 0) {
+    Report("  location: name=%s, %d\n", info.file, info.line);
   }
 }
 
 static u32 FindRegistrationSite(const Global *g) {
   mu_for_globals.CheckLocked();
   CHECK(global_registration_site_vector);
   for (uptr i = 0, n = global_registration_site_vector->size(); i < n; i++) {
     GlobalRegistrationSite &grs = (*global_registration_site_vector)[i];
for (uptr p = g.beg; p < g.beg + g.size - 1; p++) {
     unsigned char c = *(unsigned char *)p;
     if (c == '\0' || !IsASCII(c)) return;
   }
   if (*(char *)(g.beg + g.size - 1) != '\0') return;
   str->append("  '%s' is ascii string '%s'\n", MaybeDemangleGlobalName(g.name),
               (char *)g.beg);
 }
 
-static const char *GlobalFilename(const __asan_global &g) {
-  const char *res = g.module_name;
-  // Prefer the filename from source location, if is available.
-  if (g.location) res = g.location->filename;
-  CHECK(res);
-  return res;
-}
-
 void PrintGlobalLocation(InternalScopedString *str, const __asan_global &g) {
-  str->append("%s", GlobalFilename(g));
-  if (!g.location) return;
-  if (g.location->line_no) str->append(":%d", g.location->line_no);
-  if (g.location->column_no) str->append(":%d", g.location->column_no);
+  DataInfo info;
+  Symbolizer::GetOrInit()->SymbolizeData(g.beg, &info);
+
+  if (info.line != 0) {
+    str->append("%s:%d", info.file, info.line);
+  } else {
+    str->append("%s", g.module_name);
+  }
 }
 
 } // namespace __asan
 
 // ---------------------- Interface ---------------- {{{1
 using namespace __asan;
 
 // Apply __asan_register_globals to all globals found in the same loaded

compiler-rt/lib/asan/asan_interface_internal.h

extern "C" {
   struct __asan_global {
     uptr beg;                // The address of the global.
     uptr size;               // The original size of the global.
     uptr size_with_redzone;  // The size with the redzone.
     const char *name;        // Name as a C string.
     const char *module_name; // Module name as a C string. This pointer is a
                              // unique identifier of a module.
     uptr has_dynamic_init;   // Non-zero if the global has dynamic initializer.
-    __asan_global_source_location *location;  // Source location of a global,
-                                              // or NULL if it is unknown.
+    uptr windows_padding;    // TODO: Figure out how to remove this padding
+                             // that's simply here to make the MSVC incremental
+                             // linker happy...
     uptr odr_indicator;      // The address of the ODR indicator symbol.
   };
 
   // These functions can be called on some platforms to find globals in the same
   // loaded image as `flag' and apply __asan_(un)register_globals to them,
   // filtering out redundant calls.
   SANITIZER_INTERFACE_ATTRIBUTE
   void __asan_register_image_globals(uptr *flag);

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp

while (true) {
     }
     if (info->file && 0 == internal_strcmp(info->file, "??")) {
       InternalFree(info->file);
       info->file = 0;
     }
   }
 }
 
-// Parses a two-line string in the following format:
+// Parses a two- or three-line string in the following format:
 //   <symbol_name>
 //   <start_address> <size>
-// Used by LLVMSymbolizer and InternalSymbolizer.
+//   <filename>:<column>
+// Used by LLVMSymbolizer and InternalSymbolizer. LLVMSymbolizer added support
+// for symbolizing the third line in D123538, but we support the older two-line
+// information as well.
 void ParseSymbolizeDataOutput(const char *str, DataInfo *info) {
   str = ExtractToken(str, "\n", &info->name);
   str = ExtractUptr(str, " ", &info->start);
   str = ExtractUptr(str, "\n", &info->size);
+  // Note: If the third line isn't present, these calls will set info.{file,
+  // line} to empty strings.
+  str = ExtractToken(str, ":", &info->file);
+  str = ExtractUptr(str, "\n", &info->line);
 }
 
 static void ParseSymbolizeFrameOutput(const char *str,
                                       InternalMmapVector<LocalInfo> *locals) {
   if (internal_strncmp(str, "??", 2) == 0)
     return;
 
   while (*str) {

compiler-rt/test/asan/TestCases/Linux/odr-violation.cpp

 // FIXME: https://code.google.com/p/address-sanitizer/issues/detail?id=316
 // XFAIL: android
 // Fails with debug checks: https://bugs.llvm.org/show_bug.cgi?id=46862
 // XFAIL: !compiler-rt-optimized && !riscv64
 //
 // We use fast_unwind_on_malloc=0 to have full unwinding even w/o frame
 // pointers. This setting is not on by default because it's too expensive.
 //
 // Different size: detect a bug if detect_odr_violation>=1
-// RUN: %clangxx_asan -DBUILD_SO=1 -fPIC -shared %s -o %dynamiclib
-// RUN: %clangxx_asan %s %ld_flags_rpath_exe -o %t-ODR-EXE
+// RUN: %clangxx_asan -g -DBUILD_SO=1 -fPIC -shared %s -o %dynamiclib
+// RUN: %clangxx_asan -g %s %ld_flags_rpath_exe -o %t-ODR-EXE
 // RUN: %env_asan_opts=fast_unwind_on_malloc=0:detect_odr_violation=1 not %run %t-ODR-EXE 2>&1 | FileCheck %s
 // RUN: %env_asan_opts=fast_unwind_on_malloc=0:detect_odr_violation=2 not %run %t-ODR-EXE 2>&1 | FileCheck %s
 // RUN: %env_asan_opts=fast_unwind_on_malloc=0:detect_odr_violation=0     %run %t-ODR-EXE 2>&1 | FileCheck %s --check-prefix=DISABLED
 // RUN: %env_asan_opts=fast_unwind_on_malloc=0                        not %run %t-ODR-EXE 2>&1 | FileCheck %s
 //
 // Same size: report a bug only if detect_odr_violation>=2.
-// RUN: %clangxx_asan -DBUILD_SO=1 -fPIC -shared %s -o %dynamiclib -DSZ=100
+// RUN: %clangxx_asan -g -DBUILD_SO=1 -fPIC -shared %s -o %dynamiclib -DSZ=100
 // RUN: %env_asan_opts=fast_unwind_on_malloc=0:detect_odr_violation=1     %run %t-ODR-EXE 2>&1 | FileCheck %s --check-prefix=DISABLED
 // RUN: %env_asan_opts=fast_unwind_on_malloc=0:detect_odr_violation=2 not %run %t-ODR-EXE 2>&1 | FileCheck %s
 // RUN: %env_asan_opts=fast_unwind_on_malloc=0                        not %run %t-ODR-EXE 2>&1 | FileCheck %s
 // RUN: echo "odr_violation:foo::ZZZ" > %t.supp
 // RUN: %env_asan_opts=fast_unwind_on_malloc=0:detect_odr_violation=2:suppressions=%t.supp  not %run %t-ODR-EXE 2>&1 | FileCheck %s
 // RUN: echo "odr_violation:foo::G" > %t.supp
 // RUN: %env_asan_opts=fast_unwind_on_malloc=0:detect_odr_violation=2:suppressions=%t.supp      %run %t-ODR-EXE 2>&1 | FileCheck %s --check-prefix=DISABLED
 // RUN: rm -f %t.supp
 //
 // Use private aliases for global variables without indicator symbol.
-// RUN: %clangxx_asan -DBUILD_SO=1 -fPIC -shared -mllvm -asan-use-private-alias %s -o %dynamiclib -DSZ=100
-// RUN: %clangxx_asan -mllvm -asan-use-private-alias %s %ld_flags_rpath_exe -o %t-ODR-EXE
+// RUN: %clangxx_asan -g -DBUILD_SO=1 -fPIC -shared -mllvm -asan-use-private-alias %s -o %dynamiclib -DSZ=100
+// RUN: %clangxx_asan -g -mllvm -asan-use-private-alias %s %ld_flags_rpath_exe -o %t-ODR-EXE
 // RUN: %env_asan_opts=fast_unwind_on_malloc=0 %run %t-ODR-EXE 2>&1 | FileCheck %s --check-prefix=DISABLED
 
 // Use private aliases for global variables: use indicator symbol to detect ODR violation.
-// RUN: %clangxx_asan -DBUILD_SO=1 -fPIC -shared -mllvm -asan-use-private-alias -mllvm -asan-use-odr-indicator  %s -o %dynamiclib -DSZ=100
-// RUN: %clangxx_asan -mllvm -asan-use-private-alias -mllvm -asan-use-odr-indicator %s %ld_flags_rpath_exe -o %t-ODR-EXE
+// RUN: %clangxx_asan -g -DBUILD_SO=1 -fPIC -shared -mllvm -asan-use-private-alias -mllvm -asan-use-odr-indicator  %s -o %dynamiclib -DSZ=100
+// RUN: %clangxx_asan -g -mllvm -asan-use-private-alias -mllvm -asan-use-odr-indicator %s %ld_flags_rpath_exe -o %t-ODR-EXE
 // RUN: %env_asan_opts=fast_unwind_on_malloc=0 not %run %t-ODR-EXE 2>&1 | FileCheck %s
 
 // Same as above but with clang switches.
-// RUN: %clangxx_asan -DBUILD_SO=1 -fPIC -shared -fsanitize-address-use-odr-indicator %s -o %dynamiclib -DSZ=100
-// RUN: %clangxx_asan -fsanitize-address-use-odr-indicator %s %ld_flags_rpath_exe -o %t-ODR-EXE
+// RUN: %clangxx_asan -g -DBUILD_SO=1 -fPIC -shared -fsanitize-address-use-odr-indicator %s -o %dynamiclib -DSZ=100
+// RUN: %clangxx_asan -g -fsanitize-address-use-odr-indicator %s %ld_flags_rpath_exe -o %t-ODR-EXE
 // RUN: %env_asan_opts=fast_unwind_on_malloc=0 not %run %t-ODR-EXE 2>&1 | FileCheck %s
 
 // GNU driver doesn't handle .so files properly.
 // REQUIRES: Clang
 
 // REQUIRES: fast-unwinder-works
 
 #ifndef SZ
 # define SZ 4
 #endif
 
 #if BUILD_SO
 namespace foo { char G[SZ]; }
 #else
 #include <stdio.h>
 namespace foo { char G[100]; }
 // CHECK: ERROR: AddressSanitizer: odr-violation
-// CHECK: size=100 'foo::G' {{.*}}odr-violation.cpp:[[@LINE-2]]:22
+// CHECK: size=100 'foo::G' {{.*}}odr-violation.cpp:[[@LINE-2]]
 // CHECK: size={{4|100}} 'foo::G'
 int main(int argc, char **argv) {
   printf("PASS: %p\n", &foo::G);
 }
 #endif
 
 // CHECK: These globals were registered at these points:
 // CHECK: {{odr-violation.cpp|ODR-EXE}}
 // CHECK: odr-violation.cpp{{$}}
 // CHECK: SUMMARY: AddressSanitizer: odr-violation: global 'foo::G' at {{.*}}odr-violation.cpp
 // DISABLED: PASS

compiler-rt/test/asan/TestCases/Windows/global_const_string_oob.cpp

 // RUN: %clang_cl_asan -Od %s -Fe%t
 // RUN: not %run %t 2>&1 | FileCheck %s
 
 #include <windows.h>
 #include <stdio.h>
 
 extern "C" const char *foo = "foobarspam";
 
 int main(void) {
   if (foo[16])
     printf("Boo\n");
   // CHECK-NOT: Boo
   // CHECK: AddressSanitizer: global-buffer-overflow on address [[ADDR:0x[0-9a-f]+]]
   // CHECK: READ of size 1 at [[ADDR]] thread T0
   // CHECK-NEXT:   {{#0 .* main .*global_const_string_oob.cpp:}}[[@LINE-5]]
-// CHECK: [[ADDR]] is located 5 bytes to the right of global variable [[STR:.*]] defined in {{'.*global_const_string_oob.cpp:7:.*' .*}} of size 11
+  // CHECK: [[ADDR]] is located 5 bytes to the right of global variable [[STR:.*]] defined in {{'.*global_const_string_oob.cpp' .*}} of size 11
   // CHECK:   [[STR]] is ascii string 'foobarspam'
   return 0;
 }
 

compiler-rt/test/asan/TestCases/global-location-nodebug.cpp

+/// Same as global-location.cpp, but without debuginfo. In a separate file to
+/// allow this test to also run on Windows (which can't be done for the
+/// debuginfo variant).
+
+// RUN: %clangxx_asan -O2 %S/global-location.cpp -o %t -Wl,-S
+// RUN: not %run %t g 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=GLOB-NO-G
+// RUN: not %run %t c 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=CLASS_STATIC-NO-G
+// RUN: not %run %t f 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=FUNC_STATIC-NO-G
+// RUN: not %run %t l 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=LITERAL-NO-G
+
+// CHECK: AddressSanitizer: global-buffer-overflow
+// CLASS_STATIC-NO-G: 0x{{.*}} is located 4 bytes to the right of global variable 'C::array' defined in '{{.*}}global-location.cpp' {{.*}} of size 40
+// GLOB-NO-G: 0x{{.*}} is located 4 bytes to the right of global variable 'global' defined in '{{.*}}global-location.cpp' {{.*}} of size 40
+// FUNC_STATIC-NO-G: 0x{{.*}} is located 4 bytes to the right of global variable 'array' defined in '{{.*}}global-location.cpp' {{.*}} of size 40
+// LITERAL-NO-G: 0x{{.*}} is located 0 bytes to the right of global variable {{.*}} defined in '{{.*}}global-location.cpp' {{.*}} of size 11
+// CHECK: SUMMARY: AddressSanitizer: global-buffer-overflow

compiler-rt/test/asan/TestCases/global-location.cpp

-// RUN: %clangxx_asan -O2 %s -o %t
+// RUN: %clangxx_asan -g -O2 %s -o %t
 // RUN: not %run %t g 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=GLOB
 // RUN: not %run %t c 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=CLASS_STATIC
 // RUN: not %run %t f 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=FUNC_STATIC
 // RUN: not %run %t l 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=LITERAL
 
+// COFF doesn't support debuginfo for globals. For the non-debuginfo tests, see global-location-nodebug.cpp.
+// XFAIL: windows-msvc
+
 // CHECK: AddressSanitizer: global-buffer-overflow
 
 #include <string.h>
 
 struct C {
   static int array[10];
+  // CLASS_STATIC:      0x{{.*}} is located 4 bytes to the right of global variable 'C::array' defined in '{{.*}}global-location.cpp:[[@LINE-1]]' {{.*}} of size 40
 };
 
 int global[10];
-// GLOB: 0x{{.*}} is located 4 bytes to the right of global variable 'global' defined in '{{.*}}global-location.cpp:[[@LINE-1]]:5' {{.*}} of size 40
+// GLOB:      0x{{.*}} is located 4 bytes to the right of global variable 'global' defined in '{{.*}}global-location.cpp:[[@LINE-1]]' {{.*}} of size 40
 int C::array[10];
-// CLASS_STATIC: 0x{{.*}} is located 4 bytes to the right of global variable 'C::array' defined in '{{.*}}global-location.cpp:[[@LINE-1]]:8' {{.*}} of size 40
 
 int main(int argc, char **argv) {
   int one = argc - 1;
   switch (argv[1][0]) {
   case 'g': return global[one * 11];
   case 'c': return C::array[one * 11];
   case 'f':
     static int array[10];
-    // FUNC_STATIC: 0x{{.*}} is located 4 bytes to the right of global variable 'array' defined in '{{.*}}global-location.cpp:[[@LINE-1]]:16' {{.*}} of size 40
+    // FUNC_STATIC:      0x{{.*}} is located 4 bytes to the right of global variable 'array' defined in '{{.*}}global-location.cpp:[[@LINE-1]]' {{.*}} of size 40
     memset(array, 0, 10);
     return array[one * 11];
   case 'l':
     const char *str = "0123456789";
-    // LITERAL: 0x{{.*}} is located 0 bytes to the right of global variable {{.*}} defined in '{{.*}}global-location.cpp:[[@LINE-1]]:23' {{.*}} of size 11
+    // LITERAL:      0x{{.*}} is located 0 bytes to the right of global variable {{.*}} defined in '{{.*}}global-location.cpp:[[@LINE-1]]' {{.*}} of size 11
     return str[one * 11];
   }
   return 0;
 }
 
 // CHECK: SUMMARY: AddressSanitizer: global-buffer-overflow