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

[Zachary Turner via Phabricator via lldb-commits]

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).


https://reviews.llvm.org/D40616