This is an archive of the discontinued LLVM Phabricator instance.

[XCOFF] support DWARF for 32-bit XCOFF for object output
ClosedPublic

Authored by shchenz on Feb 22 2021, 12:48 AM.

Details

Summary

This patch adds DWARF support for XCOFF when clang generates object output. For now, it only works for 32-bit XCOFF.

D95518 adds DWARF support for XCOFF when clang generates assembly output.

Diff Detail

Event Timeline

shchenz created this revision.Feb 22 2021, 12:48 AM
shchenz requested review of this revision.Feb 22 2021, 12:48 AM
Herald added a project: Restricted Project. · View Herald Transcript
shchenz planned changes to this revision.Mar 8 2021, 8:39 PM
shchenz updated this revision to Diff 329248.Mar 9 2021, 1:18 AM

rebase and some formatting change

gentle ping

shchenz planned changes to this revision.Mar 9 2021, 6:02 PM
shchenz updated this revision to Diff 329556.Mar 10 2021, 12:15 AM

rebase due to change in D97049

FYI, I decide to review this patch before D97049 because I wouldn't know if the NFC make sense without the context of this patch.

llvm/lib/MC/XCOFFObjectWriter.cpp
231–232

Would SmallVector work here? I think SmallVector is generally more preferable then std::vector in llvm source.
Also, is it feasible to have pre-defined DWARF section entries as well? so that we don't need to worry about poping dwarf section when resetting. And it would be consistent with CsectSectionEntrys as well.

324

This reset for Sections is not clean. For any DwarfSectionEntrys, you only deleted what's in there. But your vector would still contain these new nullptrs.

422–424

Not a fan of raw news, it's always hard to track the ownership and where the deletion happens. I would suggest to use unique_ptr or just don't use pointer at all if possible.

590–591

Please address all the Lint comments.

678

I think we should use llvm-readobj to actually read the symbol entry for testing here.

781
1058
1060

It seems that it's possible for this address and the RawPointer to have mismatched address? As at here, we are aligning to MCSec->getAlignment(). But below when we get to dwarf sections, we would align the RawPointer to DefaultSectionAlign?
I think this is a bit confusing at least.

1085

This is more of a question:
I noticed that when doing csect calculations above, we would consider the padding as part of csect's size. But we do not consider the padding for dwarf sections to be part of the dwarf sections. Is it the behavior we derived from system assembler? Would it work if we just treat the paddings as part of dwarf sections?

llvm/test/DebugInfo/XCOFF/empty.ll
8

Could we also add a llvm-objdump test for this?
I think that test is useful to show the relationship between the dwarf raw datas and the other csects, as well as their alignments.

shchenz updated this revision to Diff 331465.Mar 17 2021, 10:41 PM
shchenz marked 8 inline comments as done.

address @jasonliu comments

@jasonliu Thanks for your comments. Please see my inline reply.

llvm/lib/MC/XCOFFObjectWriter.cpp
231–232

Yeah, SmallVector should work.

For pre-defined DWARF sections, to be honest, I don't think it is a good idea. We should let the caller of the object writer tell the writer what sections we should write. In this way, we don't need to change the writer part when we add new DWARF/Csect sections later, for example when we add DWARF 5 sections. There are many of them, 20 without split sections and 30 with split sections. It would be not good to predefined all of them.

I think maybe the current DWARF section handling should make more sense? We may need to add a FIXME to the original TEXT/DATA/BSS Csect handling?

324

Good catch.

422–424

Yeah, this is a side effect due to the inconsistent DwarfSectionEntry and CsectSectionEntry handling.
Because CsectSectionEntry are now all pre-defined as XCOFFObjectWriter's class members, we can not use unique_ptr as element of XCOFFObjectWriter::Sections.
And because there are two child types of the Sections, so we must use the parent pointer as the element of XCOFFObjectWriter::Sections.

So that seems to leave us no other choice but to use raw new.
I think if we handle DwarfSectionEntry and CsectSectionEntry the same, predefining or dynamic allocating, we should get rid of this issue.

What do you think?

1060

Yeah, agree this will cause some issue. I update the RawPointer calculation logic.

1085

We can not treat the padding as part of the DWARF sections because in some DWARF sections, like .dwline/.dwinfo section, they will contain the section size info in the section content. I think we can not make the size in DWARF section and in section header table mismatch. (Please note there is 4-byte difference for 32-bit XCOFF due to the length field itself.)

llvm/test/DebugInfo/XCOFF/empty.ll
8

I add one cases containing two cases: one is for sections dump and one is for relocation table dump. I can not dump the symbol table as now all tools don't support that. Hope this is ok.

jasonliu added inline comments.Mar 19 2021, 2:05 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
231–232

We choose to have current way of handling TEXT/DATA/BSS because we want to group certain csects together, and we don't want to use raw new/delete whenever possible. Therefore, we use the pre-defined sections.
For dwarf, do we need to group things together? If not, would it be better to treat dwarf sections as something special? For example, leave Sections as is. But create a new SmallVector<DwarfSectionEntry> DwarfSects. Notice that I'm not using pointers here, so no more raw new/delete as well. I don't see a huge value of putting csect sections and dwarf sections into the same Array, other than saving a few extra for loops.

328

This technically don't work as well, because you would clear the predefined csect sections.

422–424

Could we save XCOFFSection instead of a pointer to XCOFFSection inside of DwarfSectionEntry? That way, you don't need to new/delete.

jasonliu added inline comments.Mar 19 2021, 2:23 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
231–232

The alternative is to change the handling of TEXT/DATA/BSS csect sections as you mentioned. But if we want to go that way, I strongly prefer to do it now instead of fixing it later. I don't think having pre-defined csects sections and pushing new dwarf sections into the same array would work well as it is.

shchenz planned changes to this revision.Mar 19 2021, 6:57 PM

thanks for your review @jasonliu
I will post an NFC patch first to make csect be allocated dynamically.

llvm/lib/MC/XCOFFObjectWriter.cpp
231–232

I prefer to use a single vector to contain DWARF sections and csect sections. We should leverage the abstraction of class SectionEntry.

Will post a patch to refactor the predefined sections handling first to make them be dynamically allocated.

328

No, I think it will do the clean-up as expected. Here we should clear all sections including predefined csect sections.

shchenz updated this revision to Diff 332951.Mar 24 2021, 6:05 AM

avoid to use raw new/delete

shchenz updated this revision to Diff 332963.Mar 24 2021, 6:31 AM

1: update due to change in D99257

gentle ping

llvm/lib/MC/MCSectionXCOFF.cpp
126

Retain the assert after returning for the DWARF section case.

shchenz updated this revision to Diff 350546.Jun 8 2021, 2:48 AM

1: keep csect predefined
2: dynamic allocating dwarf sections

shchenz marked an inline comment as done.Jun 21 2021, 6:39 PM

gentle ping

gentle ping

jsji added inline comments.Jun 30 2021, 9:27 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
499

This is now a complicated expression, maybe using plain if else will make it clear without comment.

1040

Do we need this? Or an assert?

1099

Assert?

shchenz updated this revision to Diff 355820.Jul 1 2021, 3:35 AM

address @jsji comments

shchenz marked 3 inline comments as done.Jul 1 2021, 3:35 AM

Thanks for your review @jsji . Updated accordingly.

gentle ping.

gentle ping

jsji accepted this revision as: jsji.Oct 6 2021, 8:26 AM
jsji retitled this revision from [XCOFF] support DWARF for XCOFF for object output to [XCOFF] support DWARF for 32-bit XCOFF for object output.

LGTM as the first prototype, let us get this in first, we can always refactor later if needed.

This revision is now accepted and ready to land.Oct 6 2021, 8:26 AM
This revision was landed with ongoing or failed builds.Oct 7 2021, 7:41 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Oct 7 2021, 8:17 PM

Looks like this breaks tests on windows: http://45.33.8.238/win/46579/step_11.txt

Please take a look and revert for now if it takes a while to fix.

Looks like this breaks tests on windows: http://45.33.8.238/win/46579/step_11.txt

Please take a look and revert for now if it takes a while to fix.

Thanks. 7c1171a0f36ac244d687cd9d3d456f8e5de58b5c is committed