[PATCH] D133030: [AIX] llvm-readobj support a new option --exception-section for xcoff object file.

Fri Sep 2 00:40:33 PDT 2022

jhenderson added a comment.

In D133030#3763938 <https://reviews.llvm.org/D133030#3763938>, @DiggerLin wrote:

> In D133030#3763317 <https://reviews.llvm.org/D133030#3763317>, @jhenderson wrote:
>
>> Rather than canned binaries, I think it wouldn't be too hard to add yaml2obj support for exception sections, so that you can create the input at test time.
>
> yes, if I do the yaml2obj first , I need a tools to decode the object file generated by yaml2obj when I add a test for the yaml2obj, but there is not now. My propose is three steps:
>
> 1. using canned binaries in this patch,
> 2. write a second patch to yaml2obj support for exception sections(using the llvm-readobj --exception-section to test the second patch).
> 3. have another patch to modify the llvm/test/tools/llvm-readobj/XCOFF/exception-section.test which use the yaml2obj to generate xcoff object with exception sections and replace the canned binaries in the test case.

I think my preferred option would be to use yaml2bj/obj2yaml to test each other, with an additional manual step for now:

1. Write yaml2obj code for the desired behaviour, and obj2yaml code which can convert the object back into YAML, which you can then check with FileCheck.
2. After running the test, use your system tools to manually check the temporary object file created by the test looks correct. If so, land this change.
3. In a second patch (this one), use yaml2obj to create your test inputs and verify that llvm-readobj can dump them correctly.
4. You could also manually verify using your system tools that the

================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/exception-section.test:9
+# CHECK:       Exception section {
+# CHECK32-NEXT:   Symbol Index: 12 (.bar)
+# CHECK64-NEXT:   Symbol Index: 6 (.bar)
----------------
DiggerLin wrote:
> jhenderson wrote:
> > I think it would be more similar to other dumping formats to print this row as:
> > `Symbol: .bar (12)`.
> according to https://www.ibm.com/docs/en/aix/7.2?topic=formats-xcoff-object-file-format#XCOFF__iua3i23ajbau
> 
> e_addr.e_symndx+
> Symbol table index for function
>  
> the value of the field is for symbol index, so putting symbol index at first and putting symbol name in brackets are reasonable.
This isn't any different to how ELF is usually dumped. For example, when dumping ELF relocations (see for example https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-readobj/ELF/relocations.test), the ELF format refers to a symbol index, but the printout displays the symbol name. In general, if a user is trying to find out information, it is more likely that they are interested in the symbol not the index of that symbol, so printing the name first is the more useful thing.

================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/invalid-exception-section.test:1
+## Test decoding a invalid exception section.
+
----------------

================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/invalid-exception-section.test:3
+
+# RUN: dd bs=1 count=186 if=%p/Inputs/exception-section.o of=%t_invalid.o
+# RUN: printf "\x68\x65\x6c\x6c" > %t_tmp.o
----------------
`dd` isn't used anywhere else in the tests as far as I can tell, which means adding it might break some users who do not have that tool. Instead, you can use python to achieve the same effect, by reading in the file and modifying the specific bytes.

(Although this is where yaml2obj is more useful)

================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/invalid-exception-section.test:4
+# RUN: dd bs=1 count=186 if=%p/Inputs/exception-section.o of=%t_invalid.o
+# RUN: printf "\x68\x65\x6c\x6c" > %t_tmp.o
+# RUN: dd bs=1 count=4  seek=186 if=%t_tmp.o of=%t_invalid.o
----------------
Use `echo` not `printf`.

================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/invalid-exception-section.test:7
+# RUN: dd bs=1 skip=190 seek=190 count=2000 if=%p/Inputs/exception-section.o of=%t_invalid.o
+# RUN: llvm-readobj --exception-section %t_invalid.o  2>&1 |\
+# RUN:   FileCheck -DFILE=%t_invalid.o %s 
----------------

================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:144
+    if (Error E = ErrOrSymbolName.takeError()) {
+      reportUniqueWarning(std::move(E));
+      return;
----------------
I don't think you've tested this warning?

================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:508-509
+
+  if (Obj.isXCOFF() && opts::XCOFFExceptionSection)
+    Dumper->printExceptionSection();
+
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Is there a reasson you're not putting this up with the other XCOFF-specific dumping option?
> If there are several options in the command line at the same time. I  want to keep the output  as order of content xcoff object file as much as possible. 
> If you do not agree with this, I can put the all the XCOFF-specific dumping option together.
Okay, your explanation makes sense, thanks. Please put it in comments somewhere in the file, e.g. "this data appears early in XCOFF files so display it first" etc.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133030