Page MenuHomePhabricator

[profile] Support for GCDA profiling in Fuchsia
AcceptedPublic

Authored by phosek on Mar 11 2019, 11:53 AM.

Details

Summary

We already support instrumentation based profiling, but some clients
like Rust still use GCDA so we need to support it as well.

Diff Detail

Event Timeline

phosek created this revision.Mar 11 2019, 11:53 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 11 2019, 11:53 AM
Herald added subscribers: llvm-commits, Restricted Project, JDevlieghere. · View Herald Transcript
davidxl added inline comments.Mar 12 2019, 9:29 AM
compiler-rt/lib/profile/GCDAProfiling.c
258

slightly prefer

#if defined(Fuchia)
// for now empty
#else /* other platforms */

Original code

#endif

453

This big chunk of platform specific code reduces readability. Perhaps wrap it into a helper function. You can even add new file and put all platform specific code in there (starting from Fuchia -- other platforms can follow later).

phosek updated this revision to Diff 206098.Jun 21 2019, 4:26 PM
phosek marked 2 inline comments as done.

The refactoring part looks ok. +vsk to double check.

vsk added a comment.Jun 24 2019, 10:43 AM

This looks ok to me. In the longer term, I think compiler-rt would benefit from borrowing/extending llvm's generic file interface to abstract over platform differences. It doesn't seem ideal to me that libprofile/xray etc. have lots of platform-specific file I/O code.

I'd like to split the platform specific aspects to separate files, e.g. GCDAProfilingPlatformUnix.c, GCDAProfilingPlatformWindows.c and GCDAProfilingPlatformFuchsia.c as suggested by @davidxl, but I'd rather do it in a follow up change.

vsk accepted this revision.Jun 24 2019, 1:32 PM

Thanks!

This revision is now accepted and ready to land.Jun 24 2019, 1:32 PM