This is an archive of the discontinued LLVM Phabricator instance.

Differential D41097

[ELF] Use shell to check the file permissions
ClosedPublic

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

Details

Reviewers
ruiu
Commits
rG17d71c014cdd: [ELF] Use shell to check the file permissions
rLLD320442: [ELF] Use shell to check the file permissions
rL320442: [ELF] Use shell to check the file permissions
Summary

This is less error-prone to mask settings than stat.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

phosek created this revision.Dec 11 2017, 3:49 PM
Herald added a subscriber: emaste. · View Herald TranscriptDec 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

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

6–7

I think you can remove FileCheck and just do

RUN: [ ! -x %t1.o ]

because lit checks its subprocesses exit codes.

9

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.
Closed by commit rL320442: [ELF] Use shell to check the file permissions (authored by phosek). · Explain WhyDec 11 2017, 4:32 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
test/
ELF/
6 lines

Diff 126464

test/ELF/file-access.s

# REQUIRES: x86, gnustat # REQUIRES: x86, gnustat
ruiuUnsubmitted
Done
ReplyInline Actions

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

ruiu: "shell" means basically "non-Windows", so I don't think you need "linux". This test should work…
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
# RUN: ld.lld -r %t.o -o %t1.o # RUN: ld.lld -r %t.o -o %t1.o
# RUN: stat -c %%A %t1.o | FileCheck --check-prefix=CHECK-RELOC %s # RUN: stat -c %%A %t1.o | FileCheck --check-prefix=CHECK-RELOC %s
# CHECK-RELOC: -rw-r--r-- # CHECK-RELOC: -rw-
# RUN: ld.lld -shared %t.o -o %t2.so # RUN: ld.lld -shared %t.o -o %t2.so
ruiuUnsubmitted
Done
ReplyInline Actions

I think you can remove FileCheck and just do

RUN: [ ! -x %t1.o ]

because lit checks its subprocesses exit codes.

ruiu: I think you can remove FileCheck and just do RUN: [ ! -x %t1.o ] because lit checks its…
# RUN: stat -c %%A %t2.so | FileCheck --check-prefix=CHECK-SHARED %s # RUN: stat -c %%A %t2.so | FileCheck --check-prefix=CHECK-SHARED %s
# CHECK-SHARED: -rwxr-xr-x # CHECK-SHARED: -rwx
ruiuUnsubmitted
Done
ReplyInline Actions

Likewise, just

RUN: [ -x %t2.so ]
ruiu: Likewise, just RUN: [ -x %t2.so ]
# RUN: ld.lld %t.o -o %t3 # RUN: ld.lld %t.o -o %t3
# RUN: stat -c %%A %t3 | FileCheck --check-prefix=CHECK-EXEC %s # RUN: stat -c %%A %t3 | FileCheck --check-prefix=CHECK-EXEC %s
# CHECK-EXEC: -rwxr-xr-x # CHECK-EXEC: -rwx
.global _start .global _start
_start: _start:
nop nop