This patch is a piece of D99360. The new ELF notes are added in clang-offload-wrapper, and llvm-readobj has to visualize them properly.
Details
Diff Detail
Event Timeline
@vzakhari you should add to the summary "Depends on DXXXXXX" to indicate which patch(es) this patch relies on. Same goes with your other reviews. Alternatively, use the Phabricator "Edit Related Revisions" option to do a similar thing. This will tie the patches into a series, and make it clear what order you anticipate landing each change in. Note that it should be possible to land each review individually as its own commit, rather than combining them into a single big blob. this might mean redoing some reviews so that common parts (e.g. BinaryFormat) land first.
llvm/test/tools/llvm-readobj/ELF/llvmompoffload-notes.test | ||
---|---|---|
1 ↗ | (On Diff #334010) | Most note tests are named note-xxx.test, I recommend you rename this to note-llvmompoffload.test. Add a brief top-level comment to this test to indicate what the test is supposed to be testing. Name the output files distinctly, based on the important difference. This makes test debugging easier, should there be any failures. Example naming: %t.64be %t.32be %t.64le %t.32le |
2 ↗ | (On Diff #334010) | You probably want GNU-style testing too? I.e: # RUN: llvm-readelf --notes %t.o | FileCheck %s --check-prefix=NOTES-GNU (or something like that) |
36–37 ↗ | (On Diff #334010) | We usually get rid of extra whitespace, so that there is a minimal amount between the key and value in yaml docs (only enough to make all values line up vertically within a block). |
49 ↗ | (On Diff #334010) | Rather than duplicate the YAML 4 times, use yaml2obj's -D option to switch on the Class/Data types. Example usage: # RUN: yaml2obj %s -o %t.64be -DBITS=64 -DENCODING=MSB # RUN: yaml2obj %s -o %t.32be -DBITS=32 -DENCODING=MSB ... --- !ELF FileHeader: Class: ELFCLASS[[BITS]] Data: ELFDATA2[[ENCODING]] ... You can see various similar examples in many of the existing llvm-readobj tests. |
Couple of nits, otherwise looks good to me.
llvm/test/tools/llvm-readobj/ELF/note-llvmompoffload.test | ||
---|---|---|
2 | Nit: In more recent llvm tool tests, we tend to use '##' for comment markers, to distinguish them from lit and FileCheck directives. | |
5 | Here and in each other case, I'd add --match-full-lines to the FileCheck call, as this will prevent other output in the Type etc fields after the checked-for value. | |
20–21 | If you go ahead with my --match-full-lines suggestion, use {{.*}} as a regex to avoid having to check the actual offset and size here. |
Nit: In more recent llvm tool tests, we tend to use '##' for comment markers, to distinguish them from lit and FileCheck directives.