This is an archive of the discontinued LLVM Phabricator instance.

[openmp][ELF] Recognize LLVM OpenMP offload specific notes
ClosedPublic

Authored by vzakhari on Mar 29 2021, 4:16 PM.

Details

Summary

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.

Diff Detail

Event Timeline

vzakhari created this revision.Mar 29 2021, 4:16 PM
vzakhari requested review of this revision.Mar 29 2021, 4:16 PM
Herald added a project: Restricted Project. · View Herald Transcript

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

vzakhari marked 4 inline comments as done.Mar 30 2021, 11:08 AM

@jhenderson, thank you for the review and useful hints!

The change looks good to me.

jhenderson accepted this revision.Mar 31 2021, 12:01 AM

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.

This revision is now accepted and ready to land.Mar 31 2021, 12:01 AM
vzakhari updated this revision to Diff 334449.Mar 31 2021, 8:13 AM
vzakhari marked 3 inline comments as done.
MaskRay accepted this revision.Mar 31 2021, 11:54 AM