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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the comments.

In D153652#4447976 <https://reviews.llvm.org/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.



================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/X86/file-permissions.test:1
+# RUN: yaml2obj %p/Inputs/common.yaml -o %t.o
+# RUN: chmod a+x %t.o
----------------
jhenderson wrote:
> It might make sense to reuse the test code from llvm-objcopy's mirror-permissions tests. See my other inline comment too.
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.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/file-permissions.test:1
+# RUN: cp %p/Inputs/dwarf.dwo %t-exe.dwo
+# RUN: chmod a+x %t-exe.dwo
----------------
jhenderson wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153652/new/

https://reviews.llvm.org/D153652