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

[avl]

Agree that it is safe to remove all_exec from the default permissions. There are currently three usages of writeToOutput API: clang include cleaner, llvm-objcopy, llvm-dwarfutil. Both llvm-objcopy and llvm-dwarfutil use FilePermissionsApplier which should apply all_exec permission if necessary. Using all_exec by default looks redundant. It was introduced by https://reviews.llvm.org/D98426

please add the test for that case.

[hokein]

add lit tests.

[hokein]

In D153652#4445285, @avl wrote:

Agree that it is safe to remove all_exec from the default permissions. There are currently three usages of writeToOutput API: clang include cleaner, llvm-objcopy, llvm-dwarfutil. Both llvm-objcopy and llvm-dwarfutil use FilePermissionsApplier which should apply all_exec permission if necessary. Using all_exec by default looks redundant. It was introduced by https://reviews.llvm.org/D98426

please add the test for that case.

Many thanks for the verification. I added two relevant tests, please take another look.

[jhenderson]

Is there anything that can be done to use gtest unit tests for this? The two lit tests are useful, but the problem with them is that if they switch to using a different approach to emitting their data, the lit tests won't cover the Support code you're actually changing.

Some commit message nits:

[llvm][Support] Don'tt set "all_exe" mode by default for file written by llvm::writeToOutput.

There's no need to include the [llvm] tag - the [Support] tag already indicates the library clearly enough IMHO. Also, typo "Don'tt" -> "Don't" and we don't usually end commit titles with a ".".

Does the clang include cleaner tool have testing that covers its usage of this API?

llvm/test/tools/llvm-dwarfutil/ELF/X86/file-permissions.test

1 ↗

(On Diff #534438)

It might make sense to reuse the test code from llvm-objcopy's mirror-permissions tests. See my other inline comment too.

llvm/test/tools/llvm-objcopy/ELF/file-permissions.test

1 ↗

(On Diff #534438)

llvm-objcopy already has testing for permissions - see mirror-permissions-*.test and respect-umask.test. It seems like a new test file would be a mistake. Furthermore, a casual inspection to me makes it seem like this behaviour is already covered (and working correctly), which makes me think that no new testing is needed for this tool, but please review the existing test and see what you think.

[hokein]

address comments

[hokein]

Thanks for the comments.

In D153652#4447976, @jhenderson wrote:

Is there anything that can be done to use gtest unit tests for this? The two lit tests are useful, but the problem with them is that if they switch to using a different approach to emitting their data, the lit tests won't cover the Support code you're actually changing.

The usages of llvm::writeOutput are in the ToolMain.cpp files, I don't see a way to unittest them.

Some commit message nits:

[llvm][Support] Don'tt set "all_exe" mode by default for file written by llvm::writeToOutput.

There's no need to include the [llvm] tag - the [Support] tag already indicates the library clearly enough IMHO. Also, typo "Don'tt" -> "Don't" and we don't usually end commit titles with a ".".

Thanks, updated.

Does the clang include cleaner tool have testing that covers its usage of this API?

No, the clang-include-cleaner binary tool just literally writes the text code to an output file, it unlikes the llvm-dwardfutil and llvm-objcopy tools which has a preserving-input-file-permissions requirement.

llvm/test/tools/llvm-dwarfutil/ELF/X86/file-permissions.test

1 ↗

(On Diff #534438)

by reusing the code, I think you mean calling the llvm-dwardfutil tool in the llvm-objcopy lit test, I'm not sure it is a good idea, calling a different tool in the llvm/test/tools/llvm-objcopy/mirror-permissions-*.test seems like violating the layer.

llvm/test/tools/llvm-objcopy/ELF/file-permissions.test

1 ↗

(On Diff #534438)

thanks! I didn't notice there is an existing one. Taking a look on these test, I think it already covers the cases we aim to test. So no need to add a new one.

[avl]

In D153652#4448157, @hokein wrote:

Thanks for the comments.

In D153652#4447976, @jhenderson wrote:

Is there anything that can be done to use gtest unit tests for this? The two lit tests are useful, but the problem with them is that if they switch to using a different approach to emitting their data, the lit tests won't cover the Support code you're actually changing.

The usages of llvm::writeOutput are in the ToolMain.cpp files, I don't see a way to unittest them.

unit test which checks llvm::writeOutput may be added to raw_ostream_test.cpp. Let`s this test check the case when "all_exec" is not set. i.e. that after default usage of llvm::writeOutput the resulting file does not have "all_exec", which is exact purpose of this patch.

[jhenderson]

llvm/test/tools/llvm-dwarfutil/ELF/X86/file-permissions.test

1 ↗

(On Diff #534438)

I meant that you could copy and adapt the existing llvm-objcopy test to create a new llvm-dwarfutil test (in the llvm-dwarfutil test folder).

[hokein]

address comments:

• add unittest for llvm::writeToOutput API
• refine the llvm-dwarfutil tool lit test

[hokein]

Thanks for the comment.

unit test which checks llvm::writeOutput may be added to raw_ostream_test.cpp. Let`s this test check the case when "all_exec" is not set. i.e. that after default usage of llvm::writeOutput the resulting file does not have "all_exec", which is exact purpose of this patch.

OK, added one.

llvm/test/tools/llvm-dwarfutil/ELF/X86/file-permissions.test

1 ↗

(On Diff #534438)

I see, adjusted the test per comment.

[avl]

llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test

2 ↗

(On Diff #534507)

llvm/unittests/Support/raw_ostream_test.cpp

499 ↗

(On Diff #534507)

!! looks a bit unclear. Probably check it in more explicit way?

EXPECT_TRUE(Perms && !(*Perms & llvm::sys::fs::all_exe));

[jhenderson]

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

llvm/unittests/Support/raw_ostream_test.cpp

500 ↗

(On Diff #534507)

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?).

[hokein]

fix the test failures on windows

[hokein]

In 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.

llvm/unittests/Support/raw_ostream_test.cpp

499 ↗

(On Diff #534507)

sure, done.

500 ↗

(On Diff #534507)

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.

[jhenderson]

llvm/unittests/Support/raw_ostream_test.cpp

500 ↗

(On Diff #534507)

This argument doesn't make much sense to me. Why are the all_read and all_write bits implementation details that shouldn't be tested when the lack of all_exe is?

This test is for testing writeToOutput. Part of writeToOutput's behaviour appears to be to create a file with the all_read and all_write bits set. Therefore, we should be testing that behaviour. As there was already one issue with the permission bits of the file this method creates, and you are directly modifiyng a test to add permissions testing, I think it's justified to request testing of the other bits.

[hokein]

compare the file-permission bits in unittest

llvm/unittests/Support/raw_ostream_test.cpp

500 ↗

(On Diff #534507)

OK, I changed the test to verify the all_read and all_write bits, please take another look.

[avl]

this LGTM. please, wait if James has any concern.

llvm/unittests/Support/raw_ostream_test.cpp

525 ↗

(On Diff #535309)

nit: comment looks a bit inconsistent as we check for read&write bits.

[hokein]

address the comment.

llvm/unittests/Support/raw_ostream_test.cpp

525 ↗

(On Diff #535309)

refined the comment.

[jhenderson]

The new test LGTM, albeit with one query: I assume umask sets some global state. When the lit unit tests are running, do the tests within the same process run sequentially, or are they in parallel at all? If the latter, there could be some thread-safety issue, although I suspect the tests are run sequentially.

[hokein]

In D153652#4459489, @jhenderson wrote:

The new test LGTM, albeit with one query: I assume umask sets some global state. When the lit unit tests are running, do the tests within the same process run sequentially, or are they in parallel at all? If the latter, there could be some thread-safety issue, although I suspect the tests are run sequentially.

Good question. For each lit test, llvm-lit will run it on a separate worker process, so we should be safe here.

llvm/lib/Support/raw_ostream.cpp

Error llvm::writeToOutput(StringRef OutputFileName,
   if (OutputFileName == "-")
     return Write(outs());
 
   if (OutputFileName == "/dev/null") {
     raw_null_ostream Out;
     return Write(Out);
   }
 
-  unsigned Mode = sys::fs::all_read | sys::fs::all_write | sys::fs::all_exe;
+  unsigned Mode = sys::fs::all_read | sys::fs::all_write;
   Expected<sys::fs::TempFile> Temp =
       sys::fs::TempFile::create(OutputFileName + ".temp-stream-%%%%%%", Mode);
   if (!Temp)
     return createFileError(OutputFileName, Temp.takeError());
 
   raw_fd_ostream Out(Temp->FD, false);
 
   if (Error E = Write(Out)) {
     if (Error DiscardError = Temp->discard())
       return joinErrors(std::move(E), std::move(DiscardError));
     return E;
   }
   Out.flush();
 
   return Temp->keep(OutputFileName);
 }