[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

Haojian Wu via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 27 05:03:16 PDT 2023

hokein added a comment.

In D153652#4451292 <https://reviews.llvm.org/D153652#4451292>, @jhenderson wrote:

> The updated unit test is failing on Windows in the pre-merge checks. Please investigate and fix as appropriate.

Good catch, thanks. I have restricted the unittest to linux only, I think it should be fine -- the behavior on windows is a bit different, the exe bit is set even you only set `read|write` bits, the `unittests/Support/Path.cpp` verifies that behavior.

Comment at: llvm/unittests/Support/raw_ostream_test.cpp:499
+  ErrorOr<llvm::sys::fs::perms> Perms = llvm::sys::fs::getPermissions(Path);
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe);
avl wrote:
> !!  looks a bit unclear. Probably check it in more explicit way?
> EXPECT_TRUE(Perms && !(*Perms & llvm::sys::fs::all_exe)); 
sure, done.

Comment at: llvm/unittests/Support/raw_ostream_test.cpp:500
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe);
jhenderson wrote:
> Here and below, rather than just checking the all_exe bit, let's check the permissions are exactly what are expected (e.g. does it have the read/write perms?). 
checking all existing bits is a bit tricky here (I tried it, then gave up):

- createTemporaryFile() creates a file with `owner_read | owner_write`
- writeToOutput() sets the written file to `all_read | all_write`

Both API don't provide a way to customize these bits, and they're internal details. We could test against them, but testing the implementation details seems subtle. And here we aim to verify the exe-bit not set by the `writeToOutput`, so I think just testing the exe-bit is not set should be enough. 

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list