This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ/z/OS] Add GOFF reader
AbandonedPublic

Authored by yusra.syeda on Oct 2 2020, 11:53 AM.

Details

Summary

This patch consists of changes required to implement the GOFF reader for z/OS. It contains a test which uses llvm-readobj to check the symbols and relocations of the provided object file.
This patch uses https://reviews.llvm.org/D88741

Diff Detail

Event Timeline

yusra.syeda created this revision.Oct 2 2020, 11:53 AM
yusra.syeda requested review of this revision.Oct 2 2020, 11:53 AM
yusra.syeda retitled this revision from goff reader to [SystemZ/z/OS] Add GOFF reader.Oct 2 2020, 12:00 PM
yusra.syeda edited the summary of this revision. (Show Details)
yusra.syeda edited reviewers, added: hubert.reinterpretcast, uweigand, Kai; removed: jhenderson.

You may consider moving llvm-objdump to a separate patch. Some parts should be tested by a unittest in unittest/Object/

llvm/include/llvm/BinaryFormat/GOFF.h
31

const -> constexpr

llvm/lib/Object/Binary.cpp
70

Don't break the continuation of coff_* file types.

llvm/lib/Object/GOFFObjectFile.cpp
261
kpn added a subscriber: kpn.Oct 2 2020, 12:56 PM
kpn added a comment.Oct 2 2020, 1:02 PM

Are you planning on supporting the case where an ED has a nonzero offset? For a general purpose dumper it matters: HLASM.

Fix formatting issues

Fix formatting issues

It looks to me like you uploaded the wrong diff. You need to upload a diff of your entire final commit that this will be committed as, when making edits to the code. If you intend to upload a series of individual commits, and want them all reviewed together, you should create separate patches for each of them and link them together using the "edit related objects" option to create a patch series.

This patch adds support for a large number of different dumping options all at once. Please break it down into separate smaller pieces. For example, your first patch could just create the GOFFDumper, and do nothing with it, the next adds section header support, the next symbol table support and so on. It is not going to be straightforward to review such a large piece of work all at once.

This update uploads the correct diff with all changes

It looks to me like you uploaded the wrong diff. You need to upload a diff of your entire final commit that this will be committed as, when making edits to the code. If you intend to upload a series of individual commits, and want them all reviewed together, you should create separate patches for each of them and link them together using the "edit related objects" option to create a patch series.

This patch adds support for a large number of different dumping options all at once. Please break it down into separate smaller pieces. For example, your first patch could just create the GOFFDumper, and do nothing with it, the next adds section header support, the next symbol table support and so on. It is not going to be straightforward to review such a large piece of work all at once.

Thanks for the review. I will break this patch down.