[PATCH] D90013: [llvm-objdump] - Rewrite malformed-archives.test to use YAML descriptions.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/malformed-archives.test:1
-// These test checks that llvm-objdump will not crash with malformed Archive
-// files.  So the check line is not all that important but the bug fixes to
-// make sure llvm-objdump is robust is what matters.
-# RUN: not llvm-objdump --macho --archive-headers \
-# RUN:   %p/Inputs/libbogus1.a \
-# RUN:   2>&1 | FileCheck -check-prefix=bogus1 %s 
+## These test checks that llvm-objdump will not crash with malformed Archive
+## files. So the check line is not all that important but the bug fixes to
----------------



================
Comment at: llvm/test/tools/llvm-objdump/malformed-archives.test:2
+## These test checks that llvm-objdump will not crash with malformed Archive
+## files. So the check line is not all that important but the bug fixes to
+## make sure llvm-objdump is robust is what matters.
----------------



================
Comment at: llvm/test/tools/llvm-objdump/malformed-archives.test:5
 
-# bogus1: libbogus1.a': truncated or malformed archive (characters in size field in archive header are not all decimal numbers: '10%' for archive member header at offset 8)
+# RUN: yaml2obj --docnum=1 %s -o %t.libbogus1.a
+# RUN: not llvm-objdump --macho --archive-headers %t.libbogus1.a 2>&1 \
----------------
I'd add comments before each test case to make it clear what it is testing.


================
Comment at: llvm/test/tools/llvm-objdump/malformed-archives.test:6-7
+# RUN: yaml2obj --docnum=1 %s -o %t.libbogus1.a
+# RUN: not llvm-objdump --macho --archive-headers %t.libbogus1.a 2>&1 \
+# RUN:  | FileCheck -check-prefix=BOGUS1 %s
 
----------------
Optional nit, here and below.


================
Comment at: llvm/test/tools/llvm-objdump/malformed-archives.test:10-14
-# RUN: not llvm-objdump --macho --archive-headers \
-# RUN:   %p/Inputs/libbogus2.a \
-# RUN:   2>&1 | FileCheck -check-prefix=bogus2 %s 
+# BOGUS1: libbogus1.a': truncated or malformed archive (characters in size field in archive header are not all decimal numbers: '10%' for archive member header at offset 8)
 
-# bogus2: libbogus2.a': truncated or malformed archive (characters in size field in archive header are not all decimal numbers: '1%' for archive member header at offset 170)
----------------
Looking at the original libbogus1.a and libbogus2.a, I think the test was testing one or both of two possible things:
1) That the first bad member is reported, if there are multiple bad members.
2) That errors in second and subsequent members are picked up, and appropriate offsets reported, even if the first member size is valid.

I think the second point, in particular the bit about the offset being correct, is probably somewhat useful.


================
Comment at: llvm/test/tools/llvm-objdump/malformed-archives.test:47
+Magic: "!<arch>\n"
+Content: "666f6f2e63202020202020202020202020202020202020202020202020202020202020202020202030202020202020203020202020202020202060"
 
----------------
A comment showing the structure here might be good. Alternatively, what happens if you change the Terminator to be something shorter? Would that be sufficient to produce the same failure?


================
Comment at: llvm/test/tools/llvm-objdump/malformed-archives.test:55
 
-# bogus6: libbogus6.a': truncated or malformed archive (name contains a leading space for archive member header at offset 96)
+# BOGUS4: libbogus4.a': truncated or malformed archive (terminator characters in archive member "@\n" not the correct "`\n" values for the archive member header for hello.c)
 
----------------
The order earlier in the file is RUN/YAML/message, but it's switched here. I don't really mind which way around it is, but it should at least be consistent.


================
Comment at: llvm/test/tools/llvm-objdump/malformed-archives.test:73
 
-# RUN: not llvm-objdump --macho --archive-headers \
-# RUN:   %p/Inputs/libbogus8.a \
-# RUN:   2>&1 | FileCheck -check-prefix=bogus8 %s 
+# BOGUS5: libbogus5.a': truncated or malformed archive (name contains a leading space for archive member header at offset 68)
+
----------------
Aside: Not really sure this should be an error.


================
Comment at: llvm/test/tools/llvm-objdump/malformed-archives.test:255-258
+# RUN: not llvm-ar tv %t.libbogus13.a \
+# RUN:   2>&1 | FileCheck -check-prefix=BOGUS13B %s 
 
+# BOGUS13B: error: truncated or malformed archive (characters in LastModified field in archive header are not all decimal numbers: '1foobar273' for the archive member header at offset 8)
----------------
Aside: This test has nothing to do with llvm-objdump. Probably wants moving into the Object or tools/llvm-ar testing.


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

https://reviews.llvm.org/D90013