[sanitizer_common] Added VS-style output for source locations

[filcab]

Summary

With this patch, we have a flag to toggle displaying source locations in
the regular style:
file:line:column

or Visual Studio style:
file(line,column)

This way, they get picked up on the Visual Studio output window and one
can double-click them to get to that file location.

[samsonov]

Why does't passing "%s(%l,%c)" in stack_trace_format work for you?

[filcab]

It's possible, but has drawbacks:

• ReportErrorSummary wouldn't pick it up (fixable), it's using "%L %F";
• "%L" wouldn't be straightforward to use if the user wanted to change the

default (the user would have to explicitly pass "%s(%l,%c)" where they want
to have the location (instead of simply "%L");

• We would have a different DEFAULT from trunk in our (internal) port,

which would add to merge work (I don't expect it to change much, of course.
But we'd need to take care of it whenever the default changed, in order to
keep up with trunk). This is not something you need to worry, of course.
I'm just adding for completeness.

Points 1 and 2 would also apply to any sanitizer running on Windows, too
(If someone were to use them with VS, it's much easier to simply double
click the output), if we decide to turn on VS-style location info output by
default on this platform.

Thanks,

Filipe

[samsonov]

Alright, I agree with rationale for this change, and VS integration is nice to have...

lib/sanitizer_common/sanitizer_stacktrace_printer.cc

119 ↗

(On Diff #26759)

Just curious, can you print file(line) or file(line,0) in VS?

119 ↗

(On Diff #26759)

I'd rather handle VS case separately

if (vs_style && line > 0 && column > 0) {
  buffer->append("%s(%d,%d)", ....);
  return;
}

BTW, what is the expected behavior if, say, vs_style is true, but column is unknown. Is it really OK to fallback to traditional source location?

lib/sanitizer_common/sanitizer_stacktrace_printer.h

55 ↗

(On Diff #26759)

I find it confusing, that you should pass strip_path_prefix and vs_style in different order, than in the function above.

[filcab]

lib/sanitizer_common/sanitizer_flags.inc

149 ↗

(On Diff #26759)

This should probably default to SANITIZER_WINDOWS instead of false.
I think it would be the option that better matches what a user of that platform would expect, no?

lib/sanitizer_common/sanitizer_stacktrace_printer.cc

119 ↗

(On Diff #26759)

Yes.

I tested these variations:

printf(__FILE__ ": hello world!\n");
printf(__FILE__ "(0,0): hello world!\n");
printf(__FILE__ "(%d,0): hello world!\n", __LINE__);
printf(__FILE__ "(%d): hello world!\n", __LINE__);
printf(__FILE__ "(%d,17): hello world!\n", __LINE__);

Both UNIX-style ("file:3:42") and the first two in that set will select the first line in the file (absence of line info).
The other three all select their respective lines.

I think the easiest option would be to do a variation of what you mentioned:

if (vs_style && line > 0) {
  buffer->append("%s(%d,%d)", ....);
  return;
} // Fallback to UNIX-style if there's no line info

(not caring about column info and printing 0 if absent)

The "prettiest" (read: less confusing output) might be:

if (vs_style && line > 0) {
  buffer->append("%s(%d", ...);
  if (column > 0)
    buffer->append(",%d)", ....);
  else
    buffer->append(")", ....);
  return;
} // Fallback to UNIX-style if there's no line info

(Only printing column info if we actually have column info)

I'm ok with either one (or even not having the else clause and splitting "%d" from ")"). Let me know which you would prefer.

lib/sanitizer_common/sanitizer_stacktrace_printer.h

55 ↗

(On Diff #26759)

Will change.

[samsonov]

lib/sanitizer_common/sanitizer_flags.inc

149 ↗

(On Diff #26759)

I'd let Reid comment on this. I don't have strong preference about Windows defaults.

lib/sanitizer_common/sanitizer_stacktrace_printer.cc

119 ↗

(On Diff #26759)

if (vs_style && line > 0) {
  buffer->append("%s(%d", ...);
  if (column > 0) buffer->append(",%d", ...);
  buffer->append(")");
  return;
}

[filcab]

Addressed Samsonov's comments.

[filcab]

Forgot to git add the header

[filcab]

Added more RenderFrame tests. ninja check-sanitizer succeeds

[samsonov]

LGTM

[rnk]

lib/sanitizer_common/sanitizer_flags.inc

149 ↗

(On Diff #26759)

Let's default this to defined(_MSC_VER). Thankfully it is very easy to override the default sanitizer options with __asan_default_options(), so I'd rather optimize for first time users of visual studio rather than people like us with big cross-platform projects.

[filcab]

Sounds good to me. But there's a detail to be dealt with.

For out of the box _testing_, having the VS style output be the default
will force us to, either:

• use a script to run the commands (doable, but ugly (except if you need

the script anyway, for remote testing. I'm ok with this))

• or change all the tests to expect one of the output styles (I don't

really like this :-) )

I would like to avoid both of those solutions.

What do you think about doing something like this:

Have options get their values in this order:

• Compiled-in defaults
• $SANITIZER_OPTIONS (new)
• __tool_default_options() (asan, ubsan, etc)
• $TOOL_OPTIONS (ASAN, UBSAN, etc)

The rationale is:

• Compiled-in defaults should be sensible defaults for the platform.
• $SANITIZER_OPTIONS could set common flags (only, no tool-specific

options) for the user/machine

• __tool_default_options() can be used in a program to override some flags

because the test/program expects this

• $TOOL_OPTIONS is a final chance for the current user, running the

program, to override.

I'm ok with not doing this, and simply waiting until it's more necessary.
For the PS4, I can just work around it and "hack" our remote run script to
always add symbolize_vs_style=false to *_OPTIONS env vars when testing. But
it wouldn't be nice for local Windows testing (non-remote), since we would
default to vs-style (if _MSC_VER was defined) in some cases.

Any ideas?

Thanks,

Filipe

compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.cc

 }
 
 #ifndef SANITIZER_GO
 void ReportErrorSummary(const char *error_type, const AddressInfo &info) {
   if (!common_flags()->print_summary)
     return;
   InternalScopedString buff(kMaxSummaryLength);
   buff.append("%s ", error_type);
-  RenderFrame(&buff, "%L %F", 0, info, common_flags()->strip_path_prefix);
+  RenderFrame(&buff, "%L %F", 0, info, common_flags()->symbolize_vs_style,
+              common_flags()->strip_path_prefix);
   ReportErrorSummary(buff.data());
 }
 #endif
 
 void LoadedModule::set(const char *module_name, uptr base_address) {
   clear();
   full_name_ = internal_strdup(module_name);
   base_address_ = base_address;

compiler-rt/trunk/lib/sanitizer_common/sanitizer_flags.inc

COMMON_FLAG(
     "Disable core dumping. By default, disable_core=1 on 64-bit to avoid "
     "dumping a 16T+ core file. Ignored on OSes that don't dump core by"
     "default and for sanitizers that don't reserve lots of virtual memory.")
 COMMON_FLAG(bool, use_madv_dontdump, true,
           "If set, instructs kernel to not store the (huge) shadow "
           "in core file.")
 COMMON_FLAG(bool, symbolize_inline_frames, true,
             "Print inlined frames in stacktraces. Defaults to true.")
+COMMON_FLAG(bool, symbolize_vs_style, false,
+            "Print file locations in Visual Studio style (e.g: "
+            " file(10,42): ...")
 COMMON_FLAG(const char *, stack_trace_format, "DEFAULT",
             "Format string used to render stack frames. "
             "See sanitizer_stacktrace_printer.h for the format description. "
             "Use DEFAULT to get default format.")
 COMMON_FLAG(bool, no_huge_pages_for_shadow, true,
             "If true, the shadow is not allowed to use huge pages. ")
 COMMON_FLAG(bool, strict_string_checks, false,
             "If set check that string arguments are properly null-terminated")

compiler-rt/trunk/lib/sanitizer_common/sanitizer_stacktrace_libcdep.cc

for (uptr i = 0; i < size && trace[i]; i++) {
     // PCs in stack traces are actually the return addresses, that is,
     // addresses of the next instructions after the call.
     uptr pc = GetPreviousInstructionPc(trace[i]);
     SymbolizedStack *frames = Symbolizer::GetOrInit()->SymbolizePC(pc);
     CHECK(frames);
     for (SymbolizedStack *cur = frames; cur; cur = cur->next) {
       frame_desc.clear();
       RenderFrame(&frame_desc, common_flags()->stack_trace_format, frame_num++,
-                  cur->info, common_flags()->strip_path_prefix);
+                  cur->info, common_flags()->symbolize_vs_style,
+                  common_flags()->strip_path_prefix);
       Printf("%s\n", frame_desc.data());
     }
     frames->ClearAll();
   }
   // Always print a trailing empty line after stack trace.
   Printf("\n");
 }
 

compiler-rt/trunk/lib/sanitizer_common/sanitizer_stacktrace_printer.h

 //   %F - if function is known to be <foo>, prints "in <foo>", possibly
 //        followed by the offset in this function, but only if source file
 //        is unknown;
 //   %S - prints file/line/column information;
 //   %L - prints location information: file/line/column, if it is known, or
 //        module+offset if it is known, or (<unknown module>) string.
 //   %M - prints module basename and offset, if it is known, or PC.
 void RenderFrame(InternalScopedString *buffer, const char *format, int frame_no,
-                 const AddressInfo &info, const char *strip_path_prefix = "",
+                 const AddressInfo &info, bool vs_style,
+                 const char *strip_path_prefix = "",
                  const char *strip_func_prefix = "");
 
 void RenderSourceLocation(InternalScopedString *buffer, const char *file,
-                          int line, int column, const char *strip_path_prefix);
+                          int line, int column, bool vs_style,
+                          const char *strip_path_prefix);
 
 void RenderModuleLocation(InternalScopedString *buffer, const char *module,
                           uptr offset, const char *strip_path_prefix);
 
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_STACKTRACE_PRINTER_H

compiler-rt/trunk/lib/sanitizer_common/sanitizer_stacktrace_printer.cc

static const char *StripFunctionName(const char *function, const char *prefix) {
   if (0 == internal_strncmp(function, prefix, prefix_len))
     return function + prefix_len;
   return function;
 }
 
 static const char kDefaultFormat[] = "    #%n %p %F %L";
 
 void RenderFrame(InternalScopedString *buffer, const char *format, int frame_no,
-                 const AddressInfo &info, const char *strip_path_prefix,
-                 const char *strip_func_prefix) {
+                 const AddressInfo &info, bool vs_style,
+                 const char *strip_path_prefix, const char *strip_func_prefix) {
   if (0 == internal_strcmp(format, "DEFAULT"))
     format = kDefaultFormat;
   for (const char *p = format; *p != '\0'; p++) {
     if (*p != '%') {
       buffer->append("%c", *p);
       continue;
     }
     p++;
case 'F':
         buffer->append("in %s",
                        StripFunctionName(info.function, strip_func_prefix));
         if (!info.file && info.function_offset != AddressInfo::kUnknown)
           buffer->append("+0x%zx", info.function_offset);
       }
       break;
     case 'S':
       // File/line information.
-      RenderSourceLocation(buffer, info.file, info.line, info.column,
+      RenderSourceLocation(buffer, info.file, info.line, info.column, vs_style,
                            strip_path_prefix);
       break;
     case 'L':
       // Source location, or module location.
       if (info.file) {
         RenderSourceLocation(buffer, info.file, info.line, info.column,
-                             strip_path_prefix);
+                             vs_style, strip_path_prefix);
       } else if (info.module) {
         RenderModuleLocation(buffer, info.module, info.module_offset,
                              strip_path_prefix);
       } else {
         buffer->append("(<unknown module>)");
       }
       break;
     case 'M':
       // Module basename and offset, or PC.
       if (info.module)
         buffer->append("(%s+%p)", StripModuleName(info.module),
                        (void *)info.module_offset);
       else
         buffer->append("(%p)", (void *)info.address);
       break;
     default:
-      Report("Unsupported specifier in stack frame format: %c (0x%zx)!\n",
-             *p, *p);
+      Report("Unsupported specifier in stack frame format: %c (0x%zx)!\n", *p,
+             *p);
       Die();
     }
   }
 }
 
 void RenderSourceLocation(InternalScopedString *buffer, const char *file,
-                          int line, int column, const char *strip_path_prefix) {
+                          int line, int column, bool vs_style,
+                          const char *strip_path_prefix) {
+  if (vs_style && line > 0) {
+    buffer->append("%s(%d", StripPathPrefix(file, strip_path_prefix), line);
+    if (column > 0)
+      buffer->append(",%d", column);
+    buffer->append(")");
+    return;
+  }
+
   buffer->append("%s", StripPathPrefix(file, strip_path_prefix));
   if (line > 0) {
     buffer->append(":%d", line);
     if (column > 0)
       buffer->append(":%d", column);
   }
 }
 
 void RenderModuleLocation(InternalScopedString *buffer, const char *module,
                           uptr offset, const char *strip_path_prefix) {
   buffer->append("(%s+0x%zx)", StripPathPrefix(module, strip_path_prefix),
                  offset);
 }
 
 }  // namespace __sanitizer

compiler-rt/trunk/lib/sanitizer_common/tests/sanitizer_stacktrace_printer_test.cc

 #include "sanitizer_common/sanitizer_stacktrace_printer.h"
 
 #include "gtest/gtest.h"
 
 namespace __sanitizer {
 
 TEST(SanitizerStacktracePrinter, RenderSourceLocation) {
   InternalScopedString str(128);
-  RenderSourceLocation(&str, "/dir/file.cc", 10, 5, "");
+  RenderSourceLocation(&str, "/dir/file.cc", 10, 5, false, "");
   EXPECT_STREQ("/dir/file.cc:10:5", str.data());
 
   str.clear();
-  RenderSourceLocation(&str, "/dir/file.cc", 11, 0, "");
+  RenderSourceLocation(&str, "/dir/file.cc", 11, 0, false, "");
   EXPECT_STREQ("/dir/file.cc:11", str.data());
 
   str.clear();
-  RenderSourceLocation(&str, "/dir/file.cc", 0, 0, "");
+  RenderSourceLocation(&str, "/dir/file.cc", 0, 0, false, "");
   EXPECT_STREQ("/dir/file.cc", str.data());
 
   str.clear();
-  RenderSourceLocation(&str, "/dir/file.cc", 10, 5, "/dir/");
+  RenderSourceLocation(&str, "/dir/file.cc", 10, 5, false, "/dir/");
   EXPECT_STREQ("file.cc:10:5", str.data());
+
+  str.clear();
+  RenderSourceLocation(&str, "/dir/file.cc", 10, 5, true, "");
+  EXPECT_STREQ("/dir/file.cc(10,5)", str.data());
+
+  str.clear();
+  RenderSourceLocation(&str, "/dir/file.cc", 11, 0, true, "");
+  EXPECT_STREQ("/dir/file.cc(11)", str.data());
+
+  str.clear();
+  RenderSourceLocation(&str, "/dir/file.cc", 0, 0, true, "");
+  EXPECT_STREQ("/dir/file.cc", str.data());
+
+  str.clear();
+  RenderSourceLocation(&str, "/dir/file.cc", 10, 5, true, "/dir/");
+  EXPECT_STREQ("file.cc(10,5)", str.data());
 }
 
 TEST(SanitizerStacktracePrinter, RenderModuleLocation) {
   InternalScopedString str(128);
   RenderModuleLocation(&str, "/dir/exe", 0x123, "");
   EXPECT_STREQ("(/dir/exe+0x123)", str.data());
 
   // Check that we strip file prefix if necessary.
TEST(SanitizerStacktracePrinter, RenderFrame) {
   info.line = 10;
   info.column = 5;
   InternalScopedString str(256);
 
   // Dump all the AddressInfo fields.
   RenderFrame(&str, "%% Frame:%n PC:%p Module:%m ModuleOffset:%o "
                     "Function:%f FunctionOffset:%q Source:%s Line:%l "
                     "Column:%c",
-              frame_no, info, "/path/to/", "function_");
+              frame_no, info, false, "/path/to/", "function_");
   EXPECT_STREQ("% Frame:42 PC:0x400000 Module:my/module ModuleOffset:0x200 "
                "Function:foo FunctionOffset:0x100 Source:my/source Line:10 "
                "Column:5",
                str.data());
   info.Clear();
   str.clear();
 
   // Test special format specifiers.
   info.address = 0x400000;
-  RenderFrame(&str, "%M", frame_no, info);
+  RenderFrame(&str, "%M", frame_no, info, false);
   EXPECT_NE(nullptr, internal_strstr(str.data(), "400000"));
   str.clear();
 
-  RenderFrame(&str, "%L", frame_no, info);
+  RenderFrame(&str, "%L", frame_no, info, false);
   EXPECT_STREQ("(<unknown module>)", str.data());
   str.clear();
 
   info.module = internal_strdup("/path/to/module");
   info.module_offset = 0x200;
-  RenderFrame(&str, "%M", frame_no, info);
+  RenderFrame(&str, "%M", frame_no, info, false);
   EXPECT_NE(nullptr, internal_strstr(str.data(), "(module+0x"));
   EXPECT_NE(nullptr, internal_strstr(str.data(), "200"));
   str.clear();
 
-  RenderFrame(&str, "%L", frame_no, info);
+  RenderFrame(&str, "%L", frame_no, info, false);
   EXPECT_STREQ("(/path/to/module+0x200)", str.data());
   str.clear();
 
   info.function = internal_strdup("my_function");
-  RenderFrame(&str, "%F", frame_no, info);
+  RenderFrame(&str, "%F", frame_no, info, false);
   EXPECT_STREQ("in my_function", str.data());
   str.clear();
 
   info.function_offset = 0x100;
-  RenderFrame(&str, "%F %S", frame_no, info);
+  RenderFrame(&str, "%F %S", frame_no, info, false);
   EXPECT_STREQ("in my_function+0x100 <null>", str.data());
   str.clear();
 
   info.file = internal_strdup("my_file");
-  RenderFrame(&str, "%F %S", frame_no, info);
+  RenderFrame(&str, "%F %S", frame_no, info, false);
   EXPECT_STREQ("in my_function my_file", str.data());
   str.clear();
 
   info.line = 10;
-  RenderFrame(&str, "%F %S", frame_no, info);
+  RenderFrame(&str, "%F %S", frame_no, info, false);
   EXPECT_STREQ("in my_function my_file:10", str.data());
   str.clear();
 
   info.column = 5;
-  RenderFrame(&str, "%S %L", frame_no, info);
+  RenderFrame(&str, "%S %L", frame_no, info, false);
   EXPECT_STREQ("my_file:10:5 my_file:10:5", str.data());
   str.clear();
 
+  RenderFrame(&str, "%S %L", frame_no, info, true);
+  EXPECT_STREQ("my_file(10,5) my_file(10,5)", str.data());
+  str.clear();
+
+  info.column = 0;
+  RenderFrame(&str, "%F %S", frame_no, info, true);
+  EXPECT_STREQ("in my_function my_file(10)", str.data());
+  str.clear();
+
+  info.line = 0;
+  RenderFrame(&str, "%F %S", frame_no, info, true);
+  EXPECT_STREQ("in my_function my_file", str.data());
+  str.clear();
+
   info.Clear();
 }
 
 }  // namespace __sanitizer

compiler-rt/trunk/lib/tsan/rtl/tsan_report.cc

void PrintStack(const ReportStack *ent) {
   if (ent == 0 || ent->frames == 0) {
     Printf("    [failed to restore the stack]\n\n");
     return;
   }
   SymbolizedStack *frame = ent->frames;
   for (int i = 0; frame && frame->info.address; frame = frame->next, i++) {
     InternalScopedString res(2 * GetPageSizeCached());
     RenderFrame(&res, common_flags()->stack_trace_format, i, frame->info,
+                common_flags()->symbolize_vs_style,
                 common_flags()->strip_path_prefix, "__interceptor_");
     Printf("%s\n", res.data());
   }
   Printf("\n");
 }
 
 static void PrintMutexSet(Vector<ReportMopMutex> const& mset) {
   for (uptr i = 0; i < mset.Size(); i++) {

compiler-rt/trunk/lib/ubsan/ubsan_diag.cc

static void renderLocation(Location Loc) {
   InternalScopedString LocBuffer(1024);
   switch (Loc.getKind()) {
   case Location::LK_Source: {
     SourceLocation SLoc = Loc.getSourceLocation();
     if (SLoc.isInvalid())
       LocBuffer.append("<unknown>");
     else
       RenderSourceLocation(&LocBuffer, SLoc.getFilename(), SLoc.getLine(),
-                           SLoc.getColumn(), common_flags()->strip_path_prefix);
+                           SLoc.getColumn(), common_flags()->symbolize_vs_style,
+                           common_flags()->strip_path_prefix);
     break;
   }
   case Location::LK_Memory:
     LocBuffer.append("%p", Loc.getMemoryLocation());
     break;
   case Location::LK_Symbolized: {
     const AddressInfo &Info = Loc.getSymbolizedStack()->info;
     if (Info.file) {
       RenderSourceLocation(&LocBuffer, Info.file, Info.line, Info.column,
+                           common_flags()->symbolize_vs_style,
                            common_flags()->strip_path_prefix);
     } else if (Info.module) {
       RenderModuleLocation(&LocBuffer, Info.module, Info.module_offset,
                            common_flags()->strip_path_prefix);
     } else {
       LocBuffer.append("%p", Info.address);
     }
     break;