Page MenuHomePhabricator

[sanitizer] Allow Fuchsia symbolizer to be reused by Myriad RTEMS
ClosedPublic

Authored by waltl on May 4 2018, 1:49 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

waltl created this revision.May 4 2018, 1:49 PM

I'd prefer to keep all the markup string constants together. Moving them all to a header would be fine, but I think it should be a separate sanitizer_symbolizer_fuchsia.h so that non-Fuchsia builds aren't using symbolizer_fuchsia.h, which is about OS-specific stuff distinct from the symbolizer markup format. But nothing in this patch actually needs the constant to be visible outside sanitizer_symbolizer_fuchsia.cc, so I'd like to see some explanation of the need for that.

I'd be very glad to see a more thorough refactoring to make the symbolizer markup an option independent of OS. I think renaming symbolizer_fuchsia* to symbolizer_markup* probably makes sense. Ideally we'd nicely support both the Fuchsia/RTEMS configs where we want to support only markup and not have OS dependencies for things like files and subprocesses the other symbolizer code has; and also make markup a runtime selectable option for symbolizer style for the other OS builds that today have several runtime selectable choices.

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_fuchsia.cc
115 ↗(On Diff #145279)

Let's make this something like SANITIZER_USE_UNWIND, i.e. more generic to what it's doing. Then you can define that to 1/0 depending on the OS defines in one of the headers.

waltl updated this revision to Diff 145804.May 8 2018, 3:41 PM

Address CR comments

waltl marked an inline comment as done.May 8 2018, 3:47 PM

I'd prefer to keep all the markup string constants together. Moving them all to a header would be fine, but I think it should be a separate sanitizer_symbolizer_fuchsia.h so that non-Fuchsia builds aren't using symbolizer_fuchsia.h, which is about OS-specific stuff distinct from the symbolizer markup format. But nothing in this patch actually needs the constant to be visible outside sanitizer_symbolizer_fuchsia.cc, so I'd like to see some explanation of the need for that.

I'd be very glad to see a more thorough refactoring to make the symbolizer markup an option independent of OS. I think renaming symbolizer_fuchsia* to symbolizer_markup* probably makes sense. Ideally we'd nicely support both the Fuchsia/RTEMS configs where we want to support only markup and not have OS dependencies for things like files and subprocesses the other symbolizer code has; and also make markup a runtime selectable option for symbolizer style for the other OS builds that today have several runtime selectable choices.

I've done the basic refactoring as suggested. Please take a look.

waltl edited the summary of this revision. (Show Details)May 8 2018, 9:12 PM
mcgrathr accepted this revision.May 9 2018, 11:43 AM
mcgrathr added a reviewer: jakehehrlich.

LGTM assuming the Fuchsia build still works (I don't see any problems in the source off hand).
@jakehehrlich will verify the Fuchsia Build.

This revision is now accepted and ready to land.May 9 2018, 11:43 AM
vitalybuka accepted this revision.May 9 2018, 12:26 PM
waltl added a comment.May 11 2018, 4:46 PM

LGTM assuming the Fuchsia build still works (I don't see any problems in the source off hand).
@jakehehrlich will verify the Fuchsia Build.

Thanks for the review. I will go ahead and submit -- let me know if there is a problem.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: Restricted Project. · View Herald TranscriptMay 11 2018, 4:56 PM
Herald added a subscriber: delcypher. · View Herald Transcript