This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by hokein on Jun 23 2023, 11:03 AM.

Diff Detail

Event Timeline

hokein created this revision.Jun 23 2023, 11:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2023, 11:03 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
hokein requested review of this revision.Jun 23 2023, 11:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2023, 11:03 AM

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 updated this revision to Diff 534438.Jun 26 2023, 12:01 AM

add lit tests.

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.

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
2

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 updated this revision to Diff 534473.Jun 26 2023, 2:51 AM

address comments

hokein retitled this revision from [llvm][Support] Don'tt set "all_exe" mode by default for file written by llvm::writeToOutput. to [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput.Jun 26 2023, 2:52 AM

Thanks for the comments.

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
2

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 added a comment.Jun 26 2023, 4:05 AM

Thanks for the comments.

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 added inline comments.Jun 26 2023, 4:17 AM
llvm/test/tools/llvm-dwarfutil/ELF/X86/file-permissions.test
2

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 updated this revision to Diff 534507.Jun 26 2023, 5:03 AM

address comments:

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

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
2

I see, adjusted the test per comment.

avl added inline comments.Jun 26 2023, 3:29 PM
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));

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 updated this revision to Diff 534917.Jun 27 2023, 5:02 AM
hokein marked 2 inline comments as done.

fix the test failures on windows

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 added inline comments.Jun 27 2023, 5:44 AM
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 updated this revision to Diff 535309.Jun 28 2023, 3:09 AM

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 added a comment.Jun 28 2023, 5:05 PM

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 updated this revision to Diff 535712.Jun 29 2023, 4:16 AM
hokein marked an inline comment as done.

address the comment.

llvm/unittests/Support/raw_ostream_test.cpp
525 ↗(On Diff #535309)

refined the comment.

jhenderson accepted this revision.Jun 29 2023, 5:50 AM

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.

This revision is now accepted and ready to land.Jun 29 2023, 5:50 AM

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.

This revision was landed with ongoing or failed builds.Jun 30 2023, 12:20 AM
This revision was automatically updated to reflect the committed changes.