This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] Add a routine to specify the mode used when creating profile dirs.
ClosedPublic

Authored by mattd on Jul 27 2018, 8:31 PM.

Details

Summary

This patch introduces llvm_profile_set_dir_mode and llvm_profile_get_dir_mode to
the compiler-rt profile API.

Originally, profile data was placed into a directory that was created with a hard-coded
mode value of 0755 (for non-win32 builds). In certain cases, it can be helpful to create
directories with a different mode other than 0755. This patch introduces set/get
routines to allow users to specify a desired mode. The default remains at 0755.

Diff Detail

Event Timeline

mattd created this revision.Jul 27 2018, 8:31 PM

Is it possible/reasonable for the test to call __llvm_profile_recursive_mkdir and then verify the mode of the created directory?
The test would have to be flagged as unsupported on Windows but that seems fine.

Is it possible/reasonable for the test to call __llvm_profile_recursive_mkdir and then verify the mode of the created directory?
The test would have to be flagged as unsupported on Windows but that seems fine.

That't totally possible, as long as it's fine to call fstat()/stat() to gather the mode value on the resulting directory. We'd also
need to get the umask() so that we can ensure that the correct value is set with respect to the user's umask.

mattd updated this revision to Diff 158144.Jul 30 2018, 5:54 PM
  • Added a test that creates a directory via __llvm_profile_recursive_mkdir and verifies the mode.
  • I intend to only commit one of the two tests, but wanted to provide a more specific/detailed test.
    • I don't know if we really want such a specific test, but it's a potential start at one.
  • We can definitely ignore windows here (since there is no mode passed to its equivalent to mkdir).
  • I don't know if there are other systems we need to ignore, e.g., android?
  • I'm still hesitant to adding the more specific test, it feels very os specific.
  • I didn't see an alternative to FileCheck's (deprecated) %T, so I wrote a simple 'dirname' routine that is not dependent on libgen.
    • I hope that is sufficient for this test case.
mattd updated this revision to Diff 158272.Jul 31 2018, 8:13 AM
  • Renamed the test output and test directory to clean up the more specific test example.
mattd updated this revision to Diff 158298.Jul 31 2018, 9:15 AM
  • Simplify the test case, let the compiler join the literals for us.
probinson accepted this revision.Jul 31 2018, 1:54 PM

Using %t with suffixes instead of %T is the preferred idiom, glad you worked that out.
The more elaborate test depends on a set of Posix-y calls, but that's fine. If there are more environments that can't handle it, the bots will find them for you. :-)
LGTM with the fancier test, once you fix the missing RUN. (The RUN prefixes are how lit decides which lines are the test script.)

test/profile/instrprof-set-dir-mode2.c
3 ↗(On Diff #158298)

This needs a RUN: in front of it, I think.

This revision is now accepted and ready to land.Jul 31 2018, 1:54 PM
This revision was automatically updated to reflect the committed changes.
mattd marked an inline comment as done.
Herald added subscribers: Restricted Project, llvm-commits. · View Herald TranscriptJul 31 2018, 4:37 PM