[lld][test] Fix file cleanup in aarch64-build-attributes.s #164396
Conversation
This test made the foolish decision to take the lit documentation at its word: https://llvm.org/docs/CommandGuide/lit.html#substitutions "%t temporary file name unique to the test" %t is in fact the **path** of a file. As suggested by the line below that describing %basename_t. This test assumed it was just the filename itself and so left a layout of: $ tree tools/lld/test/ tools/lld/test/ ├── CMakeFiles ├── ELF │ └── Output │ ├── aarch64-build-attributes.s.tmp │ │ ├── pauth-bti-gcs.s │ │ └── pauth-bti-pac.s │ ├── aarch64-build-attributes.s.tmp.merged.o │ ├── aarch64-build-attributes.s.tmp1.o │ ├── aarch64-build-attributes.s.tmp2.o │ └── aarch64-build-attributes.s.tmp3.o ├── Unit │ └── lit.site.cfg.py ├── cmake_install.cmake └── lit.site.cfg.py Note how the 2 .s files are in the temp dir but the .o files are not. To fix this, remove %t from all the temp file names so they are created in the temp dir, which is cleaned before each run. New layout: $ tree tools/lld/test/ tools/lld/test/ ├── CMakeFiles ├── ELF │ └── Output │ └── aarch64-build-attributes.s.tmp │ ├── 1.o │ ├── 2.o │ ├── 3.o │ ├── merged.o │ ├── pauth-bti-gcs.s │ └── pauth-bti-pac.s ├── Unit │ └── lit.site.cfg.py ├── cmake_install.cmake └── lit.site.cfg.py
|
@llvm/pr-subscribers-lld-elf @llvm/pr-subscribers-lld Author: David Spickett (DavidSpickett) ChangesThis test made the foolish decision to take the lit documentation at its word: "%t temporary file name unique to the test" %t is in fact the path of a file. As suggested by the line below that describing %basename_t. This test assumed it was just the filename itself and so left a layout of: Note how the 2 .s files are in the temp dir but the .o files are not. To fix this, remove %t from all the temp file names so they are created in the temp dir, which is cleaned before each run. New layout: Full diff: https://github.com/llvm/llvm-project/pull/164396.diff 1 Files Affected:
diff --git a/lld/test/ELF/aarch64-build-attributes.s b/lld/test/ELF/aarch64-build-attributes.s
index f2d542150897e..3d333bf6ccf2f 100644
--- a/lld/test/ELF/aarch64-build-attributes.s
+++ b/lld/test/ELF/aarch64-build-attributes.s
@@ -1,11 +1,11 @@
// REQUIRES: aarch64
// RUN: rm -rf %t && split-file %s %t && cd %t
-// RUN: llvm-mc -triple=aarch64 -filetype=obj %s -o %t1.o
-// RUN: llvm-mc -triple=aarch64 -filetype=obj pauth-bti-gcs.s -o %t2.o
-// RUN: llvm-mc -triple=aarch64 -filetype=obj pauth-bti-pac.s -o %t3.o
-// RUN: ld.lld -r %t1.o %t2.o %t3.o -o %t.merged.o
-// RUN: llvm-readelf -n %t.merged.o | FileCheck %s --check-prefix=NOTE
+// RUN: llvm-mc -triple=aarch64 -filetype=obj %s -o 1.o
+// RUN: llvm-mc -triple=aarch64 -filetype=obj pauth-bti-gcs.s -o 2.o
+// RUN: llvm-mc -triple=aarch64 -filetype=obj pauth-bti-pac.s -o 3.o
+// RUN: ld.lld -r 1.o 2.o 3.o -o merged.o
+// RUN: llvm-readelf -n merged.o | FileCheck %s --check-prefix=NOTE
/// This test merges three object files with AArch64 build attributes.
/// All contain identical PAuth ABI info (platform/version), which must be preserved.
|
|
Will fix the lit docs in another PR. |
%t is currently documented as: temporary file name unique to the test Which I take to mean if the path is a/b/c/tempfile, then %t would be tempfile. It is not, it's the whole path. (which is hinted at by %basename_t, but why would you read that if you didn't need to use it) As seen in llvm#164396 this can create confusion when people use it as if it were just the file name. Make it clear in the docs that this is the path of a file.
I'm not sure if I'd make the same conclusion that this was a mistaken conclusion in itself though - it is fairly common among LLD tests to produce files like That said, I don't mind this change itself, but I wouldn't consider the previous form erroneous either - I would expect to see many similar cases among other LLD tests. |
|
The inconsistency is the problem yes. That's the bit that will waste my time if I have to debug this test again. Doing either is fine but tests should pick one or document why they need both. Not that I'm gonna hunt down the others but that's the principle. |
And this is missing the British humour context of "(and such fools include myself)", so I've changed that. If I had written this test I wouldn't have noticed this either. |
%t is currently documented as: temporary file name unique to the test https://llvm.org/docs/CommandGuide/lit.html#substitutions Which I take to mean if the path is a/b/c/tempfile, then %t would be tempfile. It is not, it's the whole path. (which is hinted at by %basename_t, but why would you read that if you didn't need to use it) As seen in #164396 this can create confusion when people use it as if it were just the file name. Make it clear in the docs that this is a unique path, which can be used to make files or folders.
%t is currently documented as: temporary file name unique to the test https://llvm.org/docs/CommandGuide/lit.html#substitutions Which I take to mean if the path is a/b/c/tempfile, then %t would be tempfile. It is not, it's the whole path. (which is hinted at by %basename_t, but why would you read that if you didn't need to use it) As seen in llvm#164396 this can create confusion when people use it as if it were just the file name. Make it clear in the docs that this is a unique path, which can be used to make files or folders.
%t is currently documented as: temporary file name unique to the test https://llvm.org/docs/CommandGuide/lit.html#substitutions Which I take to mean if the path is a/b/c/tempfile, then %t would be tempfile. It is not, it's the whole path. (which is hinted at by %basename_t, but why would you read that if you didn't need to use it) As seen in llvm#164396 this can create confusion when people use it as if it were just the file name. Make it clear in the docs that this is a unique path, which can be used to make files or folders.
%t is currently documented as: temporary file name unique to the test https://llvm.org/docs/CommandGuide/lit.html#substitutions Which I take to mean if the path is a/b/c/tempfile, then %t would be tempfile. It is not, it's the whole path. (which is hinted at by %basename_t, but why would you read that if you didn't need to use it) As seen in llvm#164396 this can create confusion when people use it as if it were just the file name. Make it clear in the docs that this is a unique path, which can be used to make files or folders.
|
@c-rhodes any comments on this? |
There was a problem hiding this comment.
i think the new layout is more sensible, so LGTM
the output of tree on the new layout in the description looks wrong tho, was this done manually? The .o files dont seem to be under the tmp directoryaarch64-build-attributes.s.tmp, like the .s files in the original layout
|
I checked locally and it was the correct layout but when rendered as markdown it normalised the spacing. With the plaintext markers the layout works and you can see all the files in the one directory. |
This test seems to have taken the lit documentation at its word: https://llvm.org/docs/CommandGuide/lit.html#substitutions "%t temporary file name unique to the test" %t is in fact the **path** of a file. As suggested by the line below that describing %basename_t. This test (I assume) assumed it was just the filename itself and so left a layout of: ``` $ tree tools/lld/test/ tools/lld/test/ ├── CMakeFiles ├── ELF │ └── Output │ ├── aarch64-build-attributes.s.tmp │ │ ├── pauth-bti-gcs.s │ │ └── pauth-bti-pac.s │ ├── aarch64-build-attributes.s.tmp.merged.o │ ├── aarch64-build-attributes.s.tmp1.o │ ├── aarch64-build-attributes.s.tmp2.o │ └── aarch64-build-attributes.s.tmp3.o ├── Unit │ └── lit.site.cfg.py ├── cmake_install.cmake └── lit.site.cfg.py ``` Note how the 2 .s files are in the temp dir but the .o files are not. This is fine, it works, but it's going to cost someone time to unpick when this test actually does fail. To fix this, remove %t from all the temp file names so they are created in the temp dir, which is cleaned before each run. New layout: ``` $ tree tools/lld/test/ tools/lld/test/ ├── CMakeFiles ├── ELF │ └── Output │ └── aarch64-build-attributes.s.tmp │ ├── 1.o │ ├── 2.o │ ├── 3.o │ ├── merged.o │ ├── pauth-bti-gcs.s │ └── pauth-bti-pac.s ├── Unit │ └── lit.site.cfg.py ├── cmake_install.cmake └── lit.site.cfg.py ```
4 participants
This test seems to have taken the lit documentation at its word:
https://llvm.org/docs/CommandGuide/lit.html#substitutions
"%t temporary file name unique to the test"
%t is in fact the path of a file. As suggested by the line below that describing %basename_t.
This test (I assume) assumed it was just the filename itself and so left a layout of:
Note how the 2 .s files are in the temp dir but the .o files are not. This is fine, it works, but it's going to cost someone time to unpick when this test actually does fail.
To fix this, remove %t from all the temp file names so they are created in the temp dir, which is cleaned before each run.
New layout: