This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Use shell to check the file permissions
ClosedPublic

Authored by phosek on Dec 11 2017, 3:49 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.Dec 11 2017, 3:49 PM
ruiu edited edge metadata.Dec 11 2017, 3:58 PM

LGTM

I believe it's still fragile against weird umask setting (such as 444), but that's in practice not an issue.

Don't you need to run this only on Unix?

ruiu added a comment.Dec 11 2017, 4:00 PM

I wonder if replacing gnustat with shell and use [ -x <filename> ] is more robust than using gnustat. At least there's a precedence in clang/test/Driver/target-override.c.

phosek updated this revision to Diff 126467.Dec 11 2017, 4:15 PM
phosek retitled this revision from [ELF] Only check user permissions in the file access test to [ELF] Use shell to check the file permissions.
phosek edited the summary of this revision. (Show Details)

Done

ruiu accepted this revision.Dec 11 2017, 4:27 PM

LGTM

test/ELF/file-access.s
1 ↗(On Diff #126467)

"shell" means basically "non-Windows", so I don't think you need "linux". This test should work fine on any Unix or on macOS.

5–6 ↗(On Diff #126467)

I think you can remove FileCheck and just do

RUN: [ ! -x %t1.o ]

because lit checks its subprocesses exit codes.

8 ↗(On Diff #126467)

Likewise, just

RUN: [ -x %t2.so ]
This revision is now accepted and ready to land.Dec 11 2017, 4:27 PM
phosek updated this revision to Diff 126473.Dec 11 2017, 4:31 PM
phosek marked 3 inline comments as done.
This revision was automatically updated to reflect the committed changes.