Page MenuHomePhabricator

[llvm-objdump] - Rewrite malformed-archives.test to use YAML descriptions.
ClosedPublic

Authored by grimar on Oct 22 2020, 11:40 PM.

Details

Summary

Currently the test uses 14 precompiled binaries. With the functionality
implemented in D89949, it is possible to remove them and use YAMLs instead.

Depends on D89949.

Diff Detail

Event Timeline

grimar created this revision.Oct 22 2020, 11:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2020, 11:40 PM
Herald added a subscriber: rupprecht. · View Herald Transcript
grimar requested review of this revision.Oct 22 2020, 11:40 PM
grimar updated this revision to Diff 300177.Oct 22 2020, 11:44 PM
  • Minor simplification.
jhenderson added inline comments.Oct 23 2020, 12:45 AM
llvm/test/tools/llvm-objdump/malformed-archives.test
1–3
2
5–6

I'd add comments before each test case to make it clear what it is testing.

6–7

Optional nit, here and below.

10–14

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.

31

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?

38–40

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.

48–79

Aside: Not really sure this should be an error.

177–180

Aside: This test has nothing to do with llvm-objdump. Probably wants moving into the Object or tools/llvm-ar testing.

grimar updated this revision to Diff 300654.Oct 26 2020, 6:42 AM
grimar marked 9 inline comments as done.
  • Addressed review comments.
  • Rebased after update of D89949.
llvm/test/tools/llvm-objdump/malformed-archives.test
10–14

I've restored that case.

31

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.

48–79

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));
...
177–180

True.

jhenderson added inline comments.Oct 27 2020, 3:16 AM
llvm/test/tools/llvm-objdump/malformed-archives.test
6

"and vice versa" implies the second case is valid (whilst the first isn't), which isn't actually true.

10

Nit. Same below.

14–16

Here and throughout, we should probably check the name in the error, message, right?

45–46

I had trouble understanding the original comment, so I've suggested an alternative.

48–79
74
87
125
138
151
164
grimar updated this revision to Diff 301201.Oct 28 2020, 2:04 AM
grimar marked 11 inline comments as done.
grimar edited the summary of this revision. (Show Details)
  • Addressed review comments.
llvm/test/tools/llvm-objdump/malformed-archives.test
45–46

Thanks.

This revision is now accepted and ready to land.Oct 28 2020, 2:46 AM