[PATCH] D99552: [openmp][ELF] Recognize LLVM OpenMP offload specific notes

[James Henderson via Phabricator via llvm-commits]

jhenderson added reviewers: MaskRay, grimar.
jhenderson added a comment.

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



================
Comment at: llvm/test/tools/llvm-readobj/ELF/llvmompoffload-notes.test:1
+# RUN: yaml2obj --docnum=1 %s -o %t.o
+# RUN: llvm-readobj --notes %t.o | FileCheck %s --check-prefix=NOTES
----------------
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`


================
Comment at: llvm/test/tools/llvm-readobj/ELF/llvmompoffload-notes.test:2
+# RUN: yaml2obj --docnum=1 %s -o %t.o
+# RUN: llvm-readobj --notes %t.o | FileCheck %s --check-prefix=NOTES
+
----------------
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)


================
Comment at: llvm/test/tools/llvm-readobj/ELF/llvmompoffload-notes.test:36-37
+Sections:
+  - Name:        .note.openmp
+    Type:        SHT_NOTE
+    Notes:
----------------
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).


================
Comment at: llvm/test/tools/llvm-readobj/ELF/llvmompoffload-notes.test:49
+
+# RUN: yaml2obj --docnum=2 %s -o %t.o
+# RUN: llvm-readobj --notes %t.o | FileCheck %s --check-prefix=NOTES
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99552