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

Mon Jun 26 01:08:53 PDT 2023

jhenderson added a comment.

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?

================
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
----------------
It might make sense to reuse the test code from llvm-objcopy's mirror-permissions tests. See my other inline comment too.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652