Page MenuHomePhabricator

[XRay] Support for Fuchsia
ClosedPublic

Authored by phosek on Sep 16 2018, 10:10 PM.

Diff Detail

Event Timeline

phosek created this revision.Sep 16 2018, 10:10 PM
dberris accepted this revision.Sep 16 2018, 11:50 PM

LGTM -- while I think there might be an opportunity for further cleaning up and isolating the platform-specific bits into different files/directories, in the meantime I suspect adding these preprocessor directives is sufficient.

This revision is now accepted and ready to land.Sep 16 2018, 11:50 PM
mcgrathr added inline comments.Sep 28 2018, 9:49 AM
compiler-rt/lib/xray/xray_allocator.h
54 ↗(On Diff #165709)

Need _zx_handle_close(Vmo); here (before the error check but after the use).

92 ↗(On Diff #165709)

Be consistent: either "VMO" or "VM object" in all messages.
s/%d/%zu/

100 ↗(On Diff #165709)

Need _zx_handle_close(Vmo); here (before the error check but after the use).

102 ↗(On Diff #165709)

s/%d/%zu/

123 ↗(On Diff #165709)

It wouldn't hurt to just define internal_munmap this way for Fuchsia if it reduces the #if load.

compiler-rt/lib/xray/xray_buffer_queue.cc
19 ↗(On Diff #165709)

These are unused in this file. Just use #if !SANITIZER_FUCHSIA around the #include "sanitizer_common/sanitizer_posix.h"

compiler-rt/lib/xray/xray_init.cc
20 ↗(On Diff #165709)

unused

compiler-rt/lib/xray/xray_interface.cc
289 ↗(On Diff #165709)

Just make MProtectHelper do this itself.

compiler-rt/lib/xray/xray_profile_collector.cc
21 ↗(On Diff #165709)

unused

compiler-rt/lib/xray/xray_utils.cc
40 ↗(On Diff #165709)

I'd use inline constexpr const char* ProfileSinkName = "llvm-xray";.
That makes the string constant mergeable.

56 ↗(On Diff #165709)

You can save the syscall when Offset % PAGE_SIZE == (Offset + TotalBytes) % PAGE_SIZE.

72 ↗(On Diff #165709)

Comment: // Nothing to do here since WriteAll writes directly into the VMO.

75 ↗(On Diff #165709)

Since this is just for the PID, I'd move it down next to where that's used and change the comment to say "Get the KOID of the current process to use in the VMO name."

81 ↗(On Diff #165709)

I think it's more useful if this message says zx_object_get_info(_zx_process_self(), ZX_INFO_HANDLE_BASIC) failed: %s.

100 ↗(On Diff #165709)

... and LogWriter needs to hold onto it.

104 ↗(On Diff #165709)

s/VMO/& handle/

108 ↗(On Diff #165709)

// Publish the VMO that receives the logging. Note the VMO's contents can grow and change after publication. The contents won't be read out until after the process exits.

112 ↗(On Diff #165709)

Put "{{{dumpfile:%s:%s}}}" into a macro in sanitizer_symbolizer_fuchsia.h and make sanitizer_coverage_fuchsia.cc and this both use it.
Unfortunately it can't be a constexpr like the others there because you need string concatenation with it.

123 ↗(On Diff #165709)

Comment after #else and #endif below.

compiler-rt/lib/xray/xray_x86_64.cc
109 ↗(On Diff #165709)

This shouldn't be here. On Fuchsia, all of this logic to read the TSC and so forth should not be used.
Instead, on all machines it should call _zx_ticks_get() to sample and _zx_ticks_per_second() to set CycleFrequency.

phosek updated this revision to Diff 169945.Oct 16 2018, 8:17 PM
phosek marked 18 inline comments as done.
phosek added inline comments.Oct 16 2018, 11:25 PM
compiler-rt/lib/xray/xray_utils.cc
40 ↗(On Diff #165709)

compiler-rt uses C++11 but inline constexpr is a C++17 construct.

mcgrathr added inline comments.Oct 17 2018, 12:25 PM
compiler-rt/lib/xray/xray_allocator.h
56 ↗(On Diff #169945)

This needs to be outside (before) the if.

106 ↗(On Diff #169945)

ditto

compiler-rt/lib/xray/xray_utils.cc
45 ↗(On Diff #169945)

This function is actually dead. Just remove it.

136 ↗(On Diff #169945)

dead

40 ↗(On Diff #165709)

Since this is not inside a class, I don't think inline actually makes a difference. The advice to use a string literal rather than a const array stands.

compiler-rt/lib/xray/xray_x86_64.cc
113 ↗(On Diff #169945)

Drop the cast. zx_ticks_t is just uint64_t and if it changed so this conversion did something, we'd want the warnings about it.
Use _zx_* names.

phosek updated this revision to Diff 170226.Oct 19 2018, 11:34 AM
phosek marked 8 inline comments as done.
mcgrathr added inline comments.Oct 19 2018, 11:51 AM
compiler-rt/lib/xray/xray_tsc.h
32 ↗(On Diff #170226)

Why not make this an inline like the clock_gettime case below does?

34 ↗(On Diff #170226)

Likewise.

compiler-rt/lib/xray/xray_utils.cc
119 ↗(On Diff #170226)

s/of/of the/

compiler-rt/lib/xray/xray_x86_64.cc
334 ↗(On Diff #170226)

This code is wrong for Fuchsia. There's nothing to do here but return true.

phosek updated this revision to Diff 170249.Oct 19 2018, 2:00 PM
phosek marked 4 inline comments as done.
Closed by commit rCRT347443: [XRay] Support for Fuchsia (authored by phosek, committed by ). · Explain WhyNov 21 2018, 6:03 PM
This revision was automatically updated to reflect the committed changes.