Page MenuHomePhabricator

[AIX][XCOFF] parsing xcoff object file auxiliary header
Needs ReviewPublic

Authored by DiggerLin on Jun 25 2020, 7:24 AM.

Details

Summary

the patch supports parsing the xcoff object file auxiliary header with llvm-readobj with option "auxiliary-headers"

the format of auxiliary header as
https://www.ibm.com/support/knowledgecenter/en/ssw_aix_72/filesreference/XCOFF.html#XCOFF__fyovh386shar

the object file
llvm/test/tools/llvm-readobj/XCOFF/Inputs/xcoff-32-xlc-exec
llvm/test/tools/llvm-readobj/XCOFF/Inputs/xcoff-32-xlc-obj.o
llvm/test/tools/llvm-readobj/XCOFF/Inputs/xcoff-64-xlc-exec
llvm/test/tools/llvm-readobj/XCOFF/Inputs/xcoff-64-xlc-obj.o

are build from the source code with xlc
struct Point {
double x;
double y;
double z;
};

Point add_points(Point a, Point b) {
Point p;
p.x = a.x + b.x;
p.y = a.y + b.y;
p.z = a.z + b.z;
return p;
}

int main() {
Point a = {1.0, 3.0, 4.0};
Point b = {2.0, 8.0, 5.0};
Point c = add_points(a, b);
return 0;
}

Diff Detail

Unit TestsFailed

TimeTest
20 mslinux > LLVM.tools/llvm-readobj/XCOFF::Unknown Unit Message ("")
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-readobj --auxiliary-headers /mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-readobj/XCOFF/Inputs/xcoff-64-xlc-exec | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck --check-prefix=XLC64EXEC /mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-readobj/XCOFF/xcoff-auxiliary-header.test
510 mslinux > MemorySanitizer-X86_64.MemorySanitizer-X86_64::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=memory -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -gline-tables-only -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/msan/strxfrm.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/X86_64/Output/strxfrm.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/X86_64/Output/strxfrm.cpp.tmp
230 mslinux > MemorySanitizer-lld-X86_64.MemorySanitizer-lld-X86_64::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=memory -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -fuse-ld=lld -gline-tables-only -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/msan/strxfrm.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/lld-X86_64/Output/strxfrm.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/lld-X86_64/Output/strxfrm.cpp.tmp
540 mslinux > SanitizerCommon-asan-x86_64-Linux.Linux::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=address -m64 -ldl -std=c++11 -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/asan-x86_64-Linux/Linux/Output/protoent.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/asan-x86_64-Linux/Linux/Output/protoent.cpp.tmp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp
300 mslinux > SanitizerCommon-lsan-x86_64-Linux.Linux::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=leak -m64 -ldl -std=c++11 -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/lsan-x86_64-Linux/Linux/Output/protoent.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/lsan-x86_64-Linux/Linux/Output/protoent.cpp.tmp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp
View Full Test Results (11 Failed)

Event Timeline

DiggerLin created this revision.Jun 25 2020, 7:24 AM
DiggerLin edited the summary of this revision. (Show Details)Jun 29 2020, 6:34 AM
jasonliu added inline comments.Wed, Jul 8, 8:42 AM
llvm/include/llvm/Object/XCOFFObjectFile.h
77

EntryPointVirtualAddr -> EntryPointAddr.

90

Why do we use char and not uint8_t?

107

Flag -> FlagAndTDataAlignment

142

X64bitsFlag -> XCOFF64Flags

229

Looks like the blank line here is for separating the functions. Why do we want to remove it?

llvm/lib/Object/XCOFFObjectFile.cpp
139

64-bit interface called on a 32-bit object file.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
107

I don't think you need to define an extra variable here.

493

Please consider combine 32 bit and 64 bit version of this function using template, as most of the fields have the same name.

500

Why do you need to pass in AuxSize to the macro function when all inputs are the same?

593

We should print this as hex.

llvm/tools/llvm-readobj/llvm-readobj.cpp
178

I'm assuming we need to add it somewhere in the llvm docs about this new option.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
490

This strikes me as extremely hazardous. What if we get a length value that is reflective of a partial field?

500

AuxSize is modified by each macro(!) invocation...

DiggerLin marked 20 inline comments as done.Thu, Jul 9, 1:15 PM
DiggerLin added inline comments.
llvm/include/llvm/Object/XCOFFObjectFile.h
90

thanks

llvm/lib/Object/XCOFFObjectFile.cpp
139

thanks

llvm/tools/llvm-readobj/XCOFFDumper.cpp
107

thanks

490

thanks

493

we print out the information based on the binary sequence of auxiliary header.
the same field is on different offset between the 32bit and 64 bits. it is difficult to implement in template.

for example.
o_tsize : Offset at 4 in 32bits , but Offset at 56 at 64bits.

500

for AuxSize is modified , I just make it looks like a function.

llvm/tools/llvm-readobj/llvm-readobj.cpp
178

thanks

DiggerLin updated this revision to Diff 276827.Thu, Jul 9, 1:34 PM
DiggerLin marked 7 inline comments as done.

address comment

llvm/tools/llvm-readobj/XCOFFDumper.cpp
488

This macro does not operate within the confines of what a function can do with respect to its caller (it can cause the caller to return early). I do not believe that using a function-like naming style is appropriate. I also do not believe that using such a macro for control flow is desirable.

You can encode a table (yes, a macro is okay for that) with much the same information:
(format, description, pointer-to-member, offset in the table past-the-end of the member)

and use that table in the place where this macro is being invoked.

490

We still have to build with C++14 compilers for the time being. Assigning a large 64-bit value to a 32-bit signed type is verboten. In any case, checking the table size against the last field of the table I described above would avoid this issue.

DiggerLin updated this revision to Diff 277057.Fri, Jul 10, 8:38 AM
DiggerLin marked 4 inline comments as done.

address comment

DiggerLin added inline comments.Fri, Jul 10, 8:39 AM
llvm/tools/llvm-readobj/XCOFFDumper.cpp
488

for the function printNumber is a overload function. using a macro, the complie will determine which version of printNumber will be used when compile. if using a table, I think how to make the code call the correct overload version of printNumber based in the parameter type when running may be complicated.

490

thanks.

hubert.reinterpretcast marked an inline comment as done.Fri, Jul 10, 12:44 PM
hubert.reinterpretcast added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
488

It can be solved using pointers-to-member in the table (I guess we don't really want to use those for selecting functions), but the macro is in better shape anyway now; thanks.

489

You can use XCOFFAuxiliaryHeader32::T in the sizeof.

499

Since the macro refers to it, move the macro definition into the scope of AuxiSize.

527

For symmetry, move the undef into the function body.

DiggerLin marked 6 inline comments as done and an inline comment as not done.Mon, Jul 13, 6:48 AM
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
489

thanks

499

thanks

527

thanks

DiggerLin updated this revision to Diff 277416.Mon, Jul 13, 6:56 AM
DiggerLin marked 3 inline comments as done.

address comment

llvm/include/llvm/Object/XCOFFObjectFile.h
142

I believe that the name @jasonliu suggested is better:
s/XCOFF64bitsFlag/XCOFF64Flags/;

llvm/tools/llvm-readobj/XCOFFDumper.cpp
504

The use of capitalization is odd. Even for title case, "of" is not capitalized in such a position.
To avoid this issue, please use lowercase following the first word (except for abbreviations or proper nouns). Please apply for all of the strings.

528

We interpreted the packed field for the symbol alignment and type in the similar case of csect auxiliary symbol table entries. For consistency within the tool, I believe we should do it here as well.

530

We should produce some warning if the auxiliary table size indicates additional content past the last field that the code understands. This is another case where I think we should also output the raw data.

548

Please indent the body of the if. Also, if AuxiSize is greater than the offset of the field but less than the offset past-the-end of the field, then we have a partial field and should probably produce some warning about it (in case the producer of the input file has behaviour we did not expect). I would also suggest to output the raw data in that case.

579

The adjective should be "64-bit". Also, these are only the additional XCOFF64 flags (and not all of the flags for XCOFF64).

Suggestion:
Additional flags for 64-bit XCOFF

jasonliu added inline comments.Mon, Jul 13, 8:42 AM
llvm/docs/CommandGuide/llvm-readobj.rst
309

nit: to match the naming convention in this file.
XCOFF SPECIFIC OPTIONS

llvm/include/llvm/Object/XCOFFObjectFile.h
142

nit: If we want to keep the bit part. Then XCOFF64BitFlag.

llvm/test/tools/llvm-readobj/XCOFF/xcoff-auxiliary-header.test
4

Should llvm-readobj --all print out the auxiliary headers?

llvm/tools/llvm-readobj/XCOFFDumper.cpp
488

nit: Address clang-format comment.

579

nit:
64 bits -> 64-bit

DiggerLin marked 6 inline comments as done.Tue, Jul 14, 7:50 AM
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
548

I agree your suggestion that "we have a partial field and should probably produce some warning about it " . If I implement the suggestion in the the patch, I also need to add test case to test it. It maybe cause the patch too big, I think it maybe better to put the suggestion in other patch.

DiggerLin updated this revision to Diff 277833.Tue, Jul 14, 7:53 AM
DiggerLin marked an inline comment as done.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
548

I would suggest implementing it in this patch and to post another patch (while this review is still up) with the specific testing for the error cases of having partial fields or trailing unrecognized fields.

DiggerLin marked 2 inline comments as done.Tue, Jul 14, 9:56 AM
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
548

without the test case , I do not think we can 100% guarantee the code is correct.
if when we write a test case for it. we find an error on the code, we would put the fix and specific test case in the some patch?

hubert.reinterpretcast marked an inline comment as done.Tue, Jul 14, 9:59 AM
hubert.reinterpretcast added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
548

Well, if the original patch has not already landed, I do not see why we wouldn't apply the fix in the original patch. As for the testing, I think it should be fine to have it in a separate patch that is intended to land at around the same time.

DiggerLin updated this revision to Diff 278168.Wed, Jul 15, 6:52 AM
DiggerLin marked an inline comment as done.

deal with partial field data

DiggerLin marked 2 inline comments as done.Wed, Jul 15, 8:55 AM
DiggerLin added inline comments.
llvm/test/tools/llvm-readobj/XCOFF/xcoff-auxiliary-header.test
4

there are several other options are not implemented, -all will be crashed.

jasonliu added inline comments.Thu, Jul 16, 11:24 AM
llvm/tools/llvm-readobj/XCOFFDumper.cpp
530

This is marked as Done. But I don't think we are able to print additional content under current revision. But anyway, I would expect this scenario would get added in the test case (which belong to another patch).

540

This field does not appear to have partial field treatment.

jasonliu added inline comments.Thu, Jul 16, 11:38 AM
llvm/include/llvm/Object/XCOFFObjectFile.h
65

This block currently cut in between XCOFFSectionHeader code block.
I would suggest to move the new block to the current line 51.
Also, is it worth to consider taking the XCOFFSectionHeader template approach now? Just like XCOFFSectionHeader, we have same member functions and share the same mask between 32 and 64 bit.

DiggerLin marked 3 inline comments as done.Thu, Jul 16, 12:10 PM
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
540

only more one bytes has partial field treatment, the flag only one byte.

DiggerLin updated this revision to Diff 278597.Thu, Jul 16, 2:07 PM
DiggerLin marked an inline comment as done.

address comment