This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt][hwasan] Add newline between record_addr lines on frame record dumps
ClosedPublic

Authored by leonardchan on Jun 7 2021, 2:48 PM.

Details

Summary

If SymbolizePC failes, it's possible for the newline to not be emitted.

Diff Detail

Event Timeline

leonardchan created this revision.Jun 7 2021, 2:48 PM
leonardchan requested review of this revision.Jun 7 2021, 2:48 PM
Herald added a subscriber: Restricted Project. · View Herald TranscriptJun 7 2021, 2:48 PM
leonardchan edited the summary of this revision. (Show Details)Jun 7 2021, 2:50 PM
vitalybuka accepted this revision.Jun 7 2021, 7:09 PM
This revision is now accepted and ready to land.Jun 7 2021, 7:09 PM
pcc requested changes to this revision.Jun 7 2021, 8:14 PM
pcc added a subscriber: pcc.

This isn't how the output looks on Android. Are you sure this isn't a Fuchsia-specific bug in the output formatting?

This revision now requires changes to proceed.Jun 7 2021, 8:14 PM

This isn't how the output looks on Android. Are you sure this isn't a Fuchsia-specific bug in the output formatting?

I don't see from the code how android can insert \n if SymbolizePC failed

This isn't how the output looks on Android. Are you sure this isn't a Fuchsia-specific bug in the output formatting?

I think the newline gets added to the frame description only if Symbolizer::GetOrInit()->SymbolizePC(pc) is nonnull, so if it fails then no newline is added.

pcc added a comment.Jun 8 2021, 12:14 PM

This isn't how the output looks on Android. Are you sure this isn't a Fuchsia-specific bug in the output formatting?

I think the newline gets added to the frame description only if Symbolizer::GetOrInit()->SymbolizePC(pc) is nonnull, so if it fails then no newline is added.

I see. I guess we never tested the code path where SymbolizePC returned null on Android.

I think I would prefer doing this in a slightly different way where the \n gets added in the Printf call on line 244. Then you can remove the \n on line 239 and you shouldn't need the else clause.

Where does the {{{bt:0:0x214178819a54}}}come from on Fuchsia? Is it printed to the console as a side effect of calling SymbolizePC?

I think the newline gets added to the frame description only if Symbolizer::GetOrInit()->SymbolizePC(pc) is nonnull, so if it fails then no newline is added.

This makes sense to me. This boils down to ListOfModules being unimplemented on Fuschia, right?

Oh, actually @pcc was right on closer inspection. This is a misunderstanding on my end. So the {{bt:...}}} comes from

constexpr const char *kFormatFrame = "{{{bt:%u:%p}}}";

defined in sanitizer_symbolizer_fuchsia.h and this is only used in the definition for RenderFrame in sanitizer_symbolizer_markup.cpp:

void RenderFrame(InternalScopedString *buffer, const char *format, int frame_no,
                 uptr address, const AddressInfo *info, bool vs_style,
                 const char *strip_path_prefix, const char *strip_func_prefix) {
  CHECK(!RenderNeedsSymbolization(format));
  buffer->append(kFormatFrame, frame_no, address);
}

and it's here where a newline isn't being added. The format isn't used at all. So it might be more appropriate to either just add a newline to those formats (or this append) or go ahead and add the newline to the Printf call on line 244.

pcc added a comment.Jun 9 2021, 10:45 AM

Thanks for tracking this down. It seems best to change the Printf call to add the newline, since otherwise it looks like you'd be adding a spurious newline to the other callers of RenderFrame on Fuchsia.

leonardchan edited the summary of this revision. (Show Details)
pcc accepted this revision.Jun 14 2021, 2:20 PM

LGTM

This revision is now accepted and ready to land.Jun 14 2021, 2:20 PM