Page MenuHomePhabricator

[llvm-objcopy] Initial XCOFF32 support.
Needs ReviewPublic

Authored by Esme on Feb 28 2021, 7:07 PM.

Details

Summary

This is an initial implementation of lvm-objcopy for XCOFF32.
Currently only supports simple copying, op-passthrough to follow.
This patch depends on D95505 because the test case requires yaml2obj.

Diff Detail

Event Timeline

Esme created this revision.Feb 28 2021, 7:07 PM
Esme requested review of this revision.Feb 28 2021, 7:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2021, 7:07 PM
MaskRay added inline comments.Mar 1 2021, 2:34 PM
llvm/test/tools/llvm-objcopy/XCOFF/basic-copy.test
26

Shouldn't the aux record follow the entry?

38

See ELF tests. We usually align the values (1 and C_EXT above, and all other values above).

Esme updated this revision to Diff 328920.Mar 7 2021, 6:56 PM

Updated the test case.

Esme added a comment.Mar 18 2021, 12:02 AM

Gently ping.

Thanks for adding objcopy support on AIX.

Some comments from me.

I have not finished all of the code reviewing yet.

llvm/include/llvm/Object/XCOFFObjectFile.h
341

I think it should be ok to call fileHeader32 directly in the Reader?

369

We already have a getSectionContents API in this class, can we reuse that function?

llvm/tools/llvm-objcopy/XCOFF/Reader.cpp
20

use &?

llvm/tools/llvm-objcopy/XCOFF/XCOFFObjcopy.cpp
40

From this comments, seems for now llvm-objcopy for XCOFF does not take any options? Even for -h or -v?
If so, I think the options list here should not be complete. For example, we miss StripDebug and there are also many others.

Maybe we need a new method to tell whether an option is specified instead of listing all of them.

Esme updated this revision to Diff 334603.Thu, Apr 1, 12:08 AM
Esme edited reviewers, added: shchenz; removed: steven.zhang.

Addressed @shchenz's comments and replace Buffe with streams based on D91028.

Esme added inline comments.Thu, Apr 1, 12:10 AM
llvm/include/llvm/Object/XCOFFObjectFile.h
341

fileHeader32() is a private member, so I added the public function getFileHeader32().

llvm/tools/llvm-objcopy/XCOFF/XCOFFObjcopy.cpp
40

I have no idea about the new method, so I chose to list all of these options, just like what other targets have done.

shchenz added a comment.EditedThu, Apr 8, 1:48 AM

Seems the functionality of the llvm-objcopy is not right. Could you please help to check? And maybe we need to add some other tests for the file format.

1:

$ llvm-objcopy 32.o copy_nodebug.o
$ dump -X32 -tv copy_nodebug.o      

copy_nodebug.o:

                        ***Symbol Table Information***
[Index] m        Value       Scn   Aux  Sclass    Type            Name
[Index] a0                                                        Fname
[Index] a1      Tagndx      Lnno  Size  Lnoptr    Endndx
[Index] a2      Tagndx            Fsiz  Lnoptr    Endndx
[Index] a3      Tagndx      Lnno  Size  Dimensions
[Index] a4   CSlen     PARMhsh SNhash SMtype SMclass Stab SNstab
[Index] a5      SECTlen    #RELent    #LINnums

[0]     m   0x00000000     undef     1  extern                    errno
[1]     a4  0x05e80000   a1f00 8192     US       ??   48 7936
[2]     m   0x00000000     undef     1  extern                    exit
[3]     a4  0x1f002000 5f80000    4     ??       PR 536872444    0
[4]     m   0x00000000     undef     1  extern                    atexit
[5]     a4  0x0000000e1f002000 1544     ER       PR 7936 8192
dump: copy_nodebug.o: dump: 0654-103 Cannot read from the specified file.
[6]     m   0x00000000     undef     1  extern                    **Invalid Symbol Name**

2: (with DWARF info)

$llvm-dwarfdump -a copy_debug.o
Program aborted due to an unhandled Error:
The end of the file was unexpectedly encountered
0.      Program arguments: llvm-dwarfdump -a copy.o
IOT/Abort trap (core dumped)
llvm/include/llvm/Object/XCOFFObjectFile.h
341

If so, I think it's better to make the private function public.

Esme added a comment.Thu, Apr 8, 2:12 AM

Good catch, thanks!

Seems the functionality of the llvm-objcopy is not right. Could you please help to check? And maybe we need to add some other tests for the file format.

1:

$ llvm-objcopy 32.o copy_nodebug.o
$ dump -X32 -tv copy_nodebug.o      
...
dump: copy_nodebug.o: dump: 0654-103 Cannot read from the specified file.
[6]     m   0x00000000     undef     1  extern                    **Invalid Symbol Name**

I will add the support for writing symbol name to StringTable in next update, which may fix the issue.

2: (with DWARF info)

$llvm-dwarfdump -a copy_debug.o
Program aborted due to an unhandled Error:
The end of the file was unexpectedly encountered
0.      Program arguments: llvm-dwarfdump -a copy.o
IOT/Abort trap (core dumped)

The patch does not support reading and writing debug info. Do you think I should add it to the patch?

The patch does not support reading and writing debug info. Do you think I should add it to the patch?

Should we distinguish the DWARF sections and the non DWARF sections for now? From the XCOFFReader and XCOFFWriter class, I can not tell why and how the DWARF sections are skipped.

Esme updated this revision to Diff 338003.Fri, Apr 16, 12:04 AM

Added the support for writing symbol names to the string table.

Esme added a comment.Fri, Apr 16, 12:08 AM

The patch does not support reading and writing debug info. Do you think I should add it to the patch?

Should we distinguish the DWARF sections and the non DWARF sections for now? From the XCOFFReader and XCOFFWriter class, I can not tell why and how the DWARF sections are skipped.

The DWARF-related failure still exists in the latest update. I will have a look into the problem.