Page MenuHomePhabricator

[UBSan] Do not overwrite the default print_summary sanitizer option.
ClosedPublic

Authored by Dor1s on Sep 4 2019, 1:53 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Dor1s created this revision.Sep 4 2019, 1:53 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 4 2019, 1:53 PM
Herald added subscribers: Restricted Project, delcypher. · View Herald Transcript
hctim added a comment.Sep 4 2019, 3:28 PM

I'm personally okay with this change, but will wait for @kcc to see what he thinks.

The current output for UBSan summaries looks like:

b.cpp:9:4: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior b.cpp:9:4 in

Can we change this to be SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior in b.cpp:9:4?

hctim added a reviewer: kcc.Sep 4 2019, 3:28 PM
Dor1s added a reviewer: vitalybuka. Dor1s removed 1 blocking reviewer(s): kcc.Sep 9 2019, 7:20 AM
Dor1s added a comment.Sep 9 2019, 8:08 AM

Removed @kcc as a "blocking" reviewer, since we've discussed this offline last week. I'll check with @vitalybuka regarding potential breakages and also ping @samsonov. Other than that, should be good to go.

@hctim regarding your question, I think the intention is that the summary line would end with the function name and/or its offset. That seems to work fine with e.g. ASan, but somehow appends an empty name for UBSan. Anyhow, it's better to be fixed in a separate change, as either of those might be reverted and landing two things together would just increase the chances.

See RenderFrame in compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp:

// Function name and offset, if file is unknown.
if (info.function) {
  buffer->append("in %s",
                 DemangleFunctionName(
                   StripFunctionName(info.function, strip_func_prefix)));
  if (!info.file && info.function_offset != AddressInfo::kUnknown)
    buffer->append("+0x%zx", info.function_offset);
}

IIRC, print_summary=false override was added to UBSan to keep its output more terse, and also to avoid essentially duplicating the information from the "runtime error" line. If the current maintainers agree that it makes sense to print summary for consistency, I have no objections.

samsonov accepted this revision.Sep 9 2019, 9:06 AM
This revision is now accepted and ready to land.Sep 9 2019, 9:06 AM
hctim accepted this revision.Sep 9 2019, 9:44 AM

SGTM. LGTM.

vitalybuka accepted this revision.Sep 9 2019, 11:04 AM

Oh, can you add some test to check output?

Dor1s updated this revision to Diff 219410.Sep 9 2019, 12:28 PM

Added a test

Dor1s added a comment.Sep 9 2019, 12:28 PM

Thanks everyone! Good point regarding the test, added!

This revision was automatically updated to reflect the committed changes.