This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer_common] Fuchsia port
AbandonedPublic

Authored by mcgrathr on Jul 25 2017, 4:02 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mcgrathr created this revision.Jul 25 2017, 4:02 PM

I split the changes strictly along subdirectory lines, but some of the stuff here only makes sense in light of the ASan port change.
Please let me know if you'd like me to slice this up into smaller pieces or recombine it and the ASan change differently.

vitalybuka added a subscriber: vitalybuka.

This one need to be split into smaller changes.

filcab requested changes to this revision.Jul 28 2017, 5:26 AM
filcab added a subscriber: filcab.
filcab added inline comments.
lib/sanitizer_common/sanitizer_common_libcdep.cc
43

In Fuchsia the output will never go into a file or somewhere where colors aren't supported?

lib/sanitizer_common/sanitizer_file.cc
20

Usual nit: sanitizer_platform.h -> Test for SANITIZER_FUCHSIA.

lib/sanitizer_common/sanitizer_fuchsia.cc
16

Assuming sanitizer_fuchsia.h has a proper guard, I'm ok with this as is, not too far from including sanitizer_platform.h only.

lib/sanitizer_common/sanitizer_fuchsia.h
16

Avoid parsing useless includes by always including sanitizer_platform.h first and testing for the platform you want.

lib/sanitizer_common/sanitizer_platform.h
279

Doesn't seem Fuchsia related.

lib/sanitizer_common/sanitizer_platform_interceptors.h
25

NOT_WINDOWS doesn't mean UNIX, though. We do have SI_POSIX, which might be close to what you need?

26

This seems highly misleading.

33

Why not #if SI_POSIX? Or maybe add some additional OS there.

120

Where did this go?

lib/sanitizer_common/sanitizer_stacktrace_printer.cc
36

Maybe have a different default? Or are you planning on simply not having this type of functionality (allow users to set their stacktrace format)?

131

Same as above.

lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc
17

Usual comment. Don't parse headers you don't need.

This revision now requires changes to proceed.Jul 28 2017, 5:26 AM

Looks good in general, but can't comment much on the OS itself :-)
I have a few changes I'd like to see, though.

mcgrathr marked 6 inline comments as done.Jul 28 2017, 2:29 PM
mcgrathr added inline comments.
lib/sanitizer_common/sanitizer_common_libcdep.cc
43

Correct. Fuchsia's logging will always go through a presentation filter that can massage the colorization as appropriate for its final output format.
I'll add a comment.

lib/sanitizer_common/sanitizer_platform.h
279

It's related in the sense that it's cleaning up an OS-specific assumption.
This setting is specifically for dealing with glibc implementation issues and hence applies only on non-Android Linux.
It so happens today that non-Android Linux is the only PPC configuration, but this setting has nothing intrinsically to do with PPC.

lib/sanitizer_common/sanitizer_platform_interceptors.h
25

It looks like SANITIZER_POSIX does in fact so far just mean !SANITIZER_WINDOWS.
So I'm fine with changing all the not-windows tests to be is-posix instead.

120

Oops. Restored.

lib/sanitizer_common/sanitizer_stacktrace_printer.cc
36

We won't have that functionality at all.
I'll add a comment.