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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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)
----------------
jhenderson wrote:
> 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.
I've restored that case.


================
Comment at: llvm/test/tools/llvm-objdump/malformed-archives.test:47
+Magic: "!<arch>\n"
+Content: "666f6f2e63202020202020202020202020202020202020202020202020202020202020202020202030202020202020203020202020202020202060"
 
----------------
jhenderson wrote:
> 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?
> Alternatively, what happens if you change the Terminator to be something shorter? Would that be sufficient to produce the same failure?

All fields, including the "Terminator" are filled to their expected length with the 0x20 (space) character.
So, no, it will not work here. We need to truncate the header of a member somehow.

I've made the sequence a bit shorter though.


================
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)
+
----------------
jhenderson wrote:
> Aside: Not really sure this should be an error.
I don't know either. But note that this only happens for BSD and DARWIN64:

```
  if (Kind == Archive::K_BSD || Kind == Archive::K_DARWIN64) {
    if (ArMemHdr->Name[0] == ' ') {
      uint64_t Offset = reinterpret_cast<const char *>(ArMemHdr) -
                        Parent->getData().data();
      return malformedError("name contains a leading space for archive member "
                            "header at offset " + Twine(Offset));
...
```


================
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)
----------------
jhenderson wrote:
> Aside: This test has nothing to do with llvm-objdump. Probably wants moving into the Object or tools/llvm-ar testing.
True.


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

https://reviews.llvm.org/D90013