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

[Pavel Labath via Phabricator via lldb-commits]

labath marked 2 inline comments as done.
labath added inline comments.


================
Comment at: lit/Modules/compressed-sections.yaml:1
+# REQUIRES: zlib
+# RUN: yaml2obj %s > %t
----------------
zturner wrote:
> 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.
llvm (and lld) also have plenty of tests ending in .yaml. Since this is a yaml file, and plenty of editors have syntax highlighting  for yaml, it seems a pitty not to take advantage of that.


================
Comment at: lit/Modules/compressed-sections.yaml:20
+  - Name:            .bogus
+# CHECK-NOT: .bogus
+    Type:            SHT_PROGBITS
----------------
zturner wrote:
> 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).
Putting CHECK-NOT first will just make sure that .bogus does not appear *before* the first CHECK match. I put it last as it this is the place it is likely to be if it did we did end up outputting it, but if we want to be safe, I guess we have two options:
- put it both at the front *and* back
- have two FileCheck invocations
I chose the latter.


https://reviews.llvm.org/D40616