[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections

Zachary Turner via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu Nov 30 09:04:20 PST 2017

zturner added inline comments.

Comment at: lit/Modules/compressed-sections.yaml:1
+# REQUIRES: zlib
+# RUN: yaml2obj %s > %t
labath wrote:
> It's right here. (I'm open to suggestions where to place it).
I see.  I think part of the reason I didn't notice it is because it has a `.yaml` extension just like the old one, so I didn't notice this was really a test.

LLVM is a little inconsistent here (it has tests that end in `.ll` and `.s`, but not for most other file extensions), so can you rename this to `compressed-sections.test`?

At some point I think we should inject another directory in this hierarchy (i.e. `lit/test/Modules`), but since this is not going to be the first directory here, I guess it doesn't need to happen now.

Comment at: lit/Modules/compressed-sections.yaml:12-13
+  - Name:            .hello_elf
+# CHECK: Section 1
+# CHECK: Name: .hello_elf
+    Type:            SHT_PROGBITS
Can you separate the `CHECK` lines and the YAML content?  I think it makes it easier to follow this way, and it gives a consistent paradigm (checks first, then input, or vice versa).  Interspersing them doesn't always work (for example if the tool doesn't output things in the same order as the input description).

Comment at: lit/Modules/compressed-sections.yaml:17-18
+    Content:         010000000800000001000000789c5330700848286898000009c802c1
+# CHECK: File size: 8
+# CHECK: Data: 2030405060708090
+  - Name:            .bogus
Can you use `CHECK-NEXT` for these two?  As it stands, if we output:

Name: .hello_elf
File size: -1
Data: -1

Name: .hello_coff
File size: 8
Data: 2030405060708090

It would pass, as written.

Comment at: lit/Modules/compressed-sections.yaml:20
+  - Name:            .bogus
+# CHECK-NOT: .bogus
+    Type:            SHT_PROGBITS
You should probably put this as the very first check statement.  Each successfully matching `CHECK` line will update an internal position and subsequent checks will only start from that position, so here you're only checking that after `.bogus` does not occur after `.hello_elf`, but this test would pass if `.bogus` occurred before `.hello_elf`.  But putting the `CHECK-NOT` first, both will fail  (this is also a good reason not to intersperse the check lines).


More information about the lldb-commits mailing list