[PATCH] D129088: [llvm-ar][test] Add testing for bitcode file handling

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-ar/bitcode-add.ll:4-5
+
+# RUN: rm -rf %t
+# RUN: split-file %s %t
+# RUN: mkdir -p %t/extracted
----------------
Fold these two into one line and then cd into them immediately before doing the mkdir and llvm-as commands, so that you can drop the %t from those commands.


================
Comment at: llvm/test/tools/llvm-ar/bitcode-add.ll:11
+## Create symtab from bitcode for a new archive.
+# RUN: llvm-ar rs new.a a.bc
+# RUN: llvm-ar t new.a | FileCheck --check-prefix=FILES %s
----------------
I'd add `c` to this command, to suppress the new archive creation warning (it's not generally an issue, but could cause confusion if a user tried to inspect the verbose output or run the command directly and started getting additional output). Ditto elsewhere in the file.


================
Comment at: llvm/test/tools/llvm-ar/bitcode-add.ll:15
+
+# FILES: a.bc
+
----------------
Should you check that there are no other files too (e.g. by adding --implicit-check-not={{.}} to your FileCheck command)?


================
Comment at: llvm/test/tools/llvm-ar/bitcode-add.ll:17-18
+
+# SYMS-NOT: ldata in {{.*}}
+# SYMS-NOT: lfunc in {{.*}}
+# SYMS: gdata in {{.*}}
----------------
This is only going to prevent ldata and lfunc appearing before gdata and gfunc. Given that you sort before the checks, this will always pass, regardless of whats in the output!

You probably want to be using `--implicit-check-not={{.}}` and check the entire contents of the output (using SYMS-NEXT as appropriate too for improved output messages).


================
Comment at: llvm/test/tools/llvm-ar/bitcode-add.ll:30
+# RUN: llvm-ar t update.a | FileCheck --check-prefix=FILES %s
+## Check no symbol table is present
+# RUN: llvm-nm --print-armap update.a | FileCheck --check-prefix=NOSYMS %s
----------------



================
Comment at: llvm/test/tools/llvm-ar/bitcode-add.ll:31
+## Check no symbol table is present
+# RUN: llvm-nm --print-armap update.a | FileCheck --check-prefix=NOSYMS %s
+# RUN: llvm-ar s update.a
----------------
What's in the output in this case? Can we check for that and use --implicit-check-not={{.}} to make the test more robust to changes in how the "Archive map" header is formatted?


================
Comment at: llvm/test/tools/llvm-ar/bitcode-add.ll:38
+
+## Update symtab from bitcode in an existing thin archive.
+# RUN: llvm-ar rS --thin update-thin.a a.bc
----------------
These "update" test cases are about creating a symbol table in an existing archive, but the test is called "bitcode-add.ll". Is the test misnamed? These update cases, aren't really "adding" anything.


================
Comment at: llvm/test/tools/llvm-ar/bitcode-add.ll:41
+# RUN: llvm-ar t update-thin.a | FileCheck --check-prefix=FILES %s
+## Check no symbol table is present
+# RUN: llvm-nm --print-armap update-thin.a | FileCheck --check-prefix=NOSYMS %s
----------------



================
Comment at: llvm/test/tools/llvm-ar/bitcode-add.ll:48
+## Create symtab from bitcode from another archive.
+# RUN: llvm-ar rS input.a a.bc
+# RUN: llvm-ar qsL lib.a input.a
----------------
Do you need the `S` here? What does suppressing the archive symtab in this case actually achieve? Ditto below.


================
Comment at: llvm/test/tools/llvm-ar/bitcode-add.ll:60-62
+# RUN: cd %t/extracted
+# RUN: llvm-ar x %t/new.a a.bc
+# RUN: diff a.bc %t/a.bc
----------------
Again, not a test case to do with "adding" anything.


================
Comment at: llvm/test/tools/llvm-ar/mri-bitcode.ll:1
+## Show that when bitcode is added to an archive via mri script it is
+## handled correctly. The symbol table is as expected and it can be
----------------
This test basically looks identical to the other test, except that MRI scripts are used to drive the program. I feel like it would be better to include the MRI cases alongside the non-MRI cases in the same test file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129088