Page MenuHomePhabricator

[llvm-symbolizer] - Fix the crash in GNU output style with --no-inlines and missing input file.
ClosedPublic

Authored by grimar on Jan 28 2021, 5:47 AM.

Details

Summary

Fixes https://bugs.llvm.org/show_bug.cgi?id=48882.

If the input file does not exist (or has a reading error), the
following code will crash if there are two or more input addresses.

auto ResOrErr = Symbolizer.symbolizeInlinedCode(
  ModuleName, {Offset, object::SectionedAddress::UndefSection});
Printer << (error(ResOrErr) ? DILineInfo() : ResOrErr.get().getFrame(0));

For the first address, symbolizeInlinedCode returns an error.
For the second address, symbolizeInlinedCode returns an empty result
(not an error) and .getFrame(0) will crash.

Diff Detail

Event Timeline

grimar created this revision.Jan 28 2021, 5:47 AM
grimar requested review of this revision.Jan 28 2021, 5:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2021, 5:47 AM
grimar edited the summary of this revision. (Show Details)Jan 28 2021, 5:47 AM
grimar added inline comments.Jan 28 2021, 5:56 AM
llvm/test/tools/llvm-symbolizer/output-style-inlined.test
39

This might need an update if D95246 will be landed first.

jhenderson added inline comments.Jan 28 2021, 6:00 AM
llvm/test/tools/llvm-symbolizer/output-style-inlined.test
32

You probably need to explain in the comment why you use --no-inlines, as it's not obvious.

MaskRay added a comment.EditedJan 28 2021, 12:05 PM

Perhaps clarify the description as following (I had had trouble understanding it until I debugged it).

If the input file does not exist (or has a reading error), the following code will crash if there are two or more input addresses.

auto ResOrErr = Symbolizer.symbolizeInlinedCode(
  ModuleName, {Offset, object::SectionedAddress::UndefSection});
Printer << (error(ResOrErr) ? DILineInfo() : ResOrErr.get().getFrame(0));

For the first address, symbolizeInlinedCode returns an error.
For the second address, symbolizeInlinedCode returns an empty result (not an error) and .getFrame(0) will crash.

I thought about whether non-first runs should return an error as well, but then I realized that the error cannot be easily serialized and stored, so returning an empty result should be fine.

grimar edited the summary of this revision. (Show Details)Jan 28 2021, 11:58 PM
grimar edited the summary of this revision. (Show Details)
grimar updated this revision to Diff 320062.Jan 29 2021, 12:07 AM
grimar marked an inline comment as done.
  • Addressed review comments.
jhenderson accepted this revision.Jan 29 2021, 12:39 AM

LGTM. I've also approved D95246. That's a larger change, so perhaps let that land first, if you can then rebase and commit. If it hasn't landed before you need to be stopping work, feel free to land this first though, given you're away soon.

This revision is now accepted and ready to land.Jan 29 2021, 12:39 AM

LGTM. I've also approved D95246. That's a larger change, so perhaps let that land first, if you can then rebase and commit. If it hasn't landed before you need to be stopping work, feel free to land this first though, given you're away soon.

Thanks! I can commit this patch during this weekends I think. Hope D95246 will be landed today and then worth giving it a bit of time to let bots to check it I guess.

MaskRay accepted this revision.Jan 29 2021, 12:44 AM

Thanks!

@MaskRay, this is another that I think could benefit being in the LLVM 12 release branch. What do you think? If so, could you make the request to do so, please, given I'll be away the rest of this week.

@MaskRay, this is another that I think could benefit being in the LLVM 12 release branch. What do you think? If so, could you make the request to do so, please, given I'll be away the rest of this week.

Sounds good. Filed https://bugs.llvm.org/show_bug.cgi?id=49227