[PATCH] D57365: [llvm-readobj] Add a flag to dump just the section-to-segment mapping.

Wed Jan 30 09:28:26 PST 2019

mattd added inline comments.

================
Comment at: llvm/test/tools/llvm-readobj/gnu-phdrs.test:17-18
+RUN:   --elf-output-style=GNU | FileCheck %s -check-prefixes ELF64-PHDRS,ELF64-MAPPING
+RUN: llvm-readelf -program-headers %p/Inputs/phdrs-elf.exe-x86_64 \
+RUN:   | FileCheck %s -check-prefixes ELF64-PHDRS,ELF64-MAPPING
+RUN: llvm-readelf -section-mapping -program-headers %p/Inputs/phdrs-elf.exe-i386 \
----------------
jhenderson wrote:
> I'm not sure you need this test case, as it is identical to the previous one essentially. `llvm-readobj --elf-output-style=GNU` and `llvm-readelf` are essentially identical (ignoring certain unrelated switch interpretations and aliases).
I understand the differences between the two tools, I just wanted to test the output.  It agree it does feel a bit redundant.

================
Comment at: llvm/test/tools/llvm-readobj/gnu-phdrs.test:19-20
+RUN:   | FileCheck %s -check-prefixes ELF64-PHDRS,ELF64-MAPPING
+RUN: llvm-readelf -section-mapping -program-headers %p/Inputs/phdrs-elf.exe-i386 \
+RUN:   | FileCheck %s -check-prefix ELF32
+
----------------
jhenderson wrote:
> What is this check giving us that's different to the one on line 13?
Since ELF32 is a single prefix,  the order of the output, the headers immediately followed by the mapping, is checked.  I want to check the case where both options `-section-mapping -program-headers` is presented,  to ensure that the headers are still displayed before the mapping.

================
Comment at: llvm/test/tools/llvm-readobj/gnu-phdrs.test:26
+RUN: llvm-readelf -section-mapping %p/Inputs/phdrs-elf.exe-x86_64 \
+RUN:   | not grep "Program Headers:"
+
----------------
jhenderson wrote:
> grimar wrote:
> > jhenderson wrote:
> > > Using grep in lit has been deprecated, I believe. Use FileCheck `CHECK-NOT` patterns.
> > > 
> > > You can do the checking that the program headers are not present at the same time as the section mapping test, by using careful placement of the NOT patterns. NOT patterns fail if the output appears between the two positive checks either side of the NOT (or the start/end of file if there are none before/after it). Example that would fail if "Program Headers:" appears before or after the section mapping:
> > > ```
> > > CHECK-NOT: Program Headers:
> > > CHECK: Mapping
> > > CHECK-NOT: Program Headers:
> > > ```
> > > You can even use `check-prefixes` to share check patterns in a mix-and-match kind of way to avoid duplicating common things. All the names get added to one big bucket. When FileCheck scans the file for patterns it uses any with a matching prefix, in sequence, regardless of which prefix (i.e. the order of prefixes in the option is irrelevant).
> > > 
> > > For example, for the following case, using `FileCheck --check-prefixes=NO-PHDRS,CHECK` would be the equivalent of the above test case, whilst `FileCheck --check-prefixes=PHDRS,CHECK`  or `FileCheck --check-prefixes=CHECK,PHDRS` would match the program headers followed by the mapping. I think you could even use `FileCheck --check-prefixes=NO-PHDRS,CHECK,PHDRS` to show that the Program Headers appear followed by the mapping, but that no more program headers appear after the mapping (I'm not quite sure whether the first NO-PHDRS-NOT would have any effect in this case though).
> > > ```
> > > NO-PHDRS-NOT: Program Headers:
> > > PHDRS: Program Headers:
> > > CHECK: Mapping
> > > NO-PHDRS-NOT: Program Headers:
> > > ```
> > > 
> > > Using grep in lit has been deprecated, I believe. Use FileCheck CHECK-NOT patterns.
> > 
> > Oh, I wanted to write the same comment at first but found that `grep` is also used in LLVM. I did not realize it is deprecated, supposed we just use both ways.
> I found the reference in the [[ https://llvm.org/docs/TestingGuide.html#writing-new-regression-tests | testing documentation ]]:
> 
> > The recommended way to examine output to figure out if the test passes is using the FileCheck tool. [The usage of grep in RUN lines is deprecated - please do not send or commit patches that use it.]
> 
> 
> When FileCheck scans the file for patterns it uses any with a matching prefix, in sequence, regardless of which prefix (i.e. the order of prefixes in the option is irrelevant).

Yep, that's what I meant in my patch description when I mentioned that the order of the prefixes is not respected.  I tested that by just swapping the order of the arguments to the --check-prefixes=PHDRS,MAPPINGS and --check-prefixes=MAPPINGS,PHDRS.

> Using grep in lit has been deprecated, I believe. Use FileCheck CHECK-NOT patterns.
Thanks for pointing the grep deprecation out! Honestly, when I was updating this patch,  I just grepped around the source code looking for instances of "RUN.*grep" to see if my run-line looked correct.  Since I found similar implementations, I ran with it.  I'll update to `CHECK-NOT`  Grepping for "grep" is rather satisfying.

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

https://reviews.llvm.org/D57365