This is an archive of the discontinued LLVM Phabricator instance.

[NFC][XCOFF][AIX] Serialize object file writing for each CsectGroup
ClosedPublic

Authored by jasonliu on Oct 17 2019, 8:37 AM.

Details

Summary

Right now we handle each CsectGroup(ProgramCodeCsects, BSSCsects) individually when assigning indices, writing symbol table, and writing section raw data. However, there is already a pattern there, and we could common up those actions for every CsectGroup. This will make adding new CsectGroup(Read Write data, Read only data, TC/TOC, mergeable string) easier, and less error prone.

Diff Detail

Event Timeline

jasonliu created this revision.Oct 17 2019, 8:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2019, 8:38 AM
jasonliu updated this revision to Diff 225446.Oct 17 2019, 9:31 AM
daltenty added inline comments.Oct 17 2019, 10:31 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
114

nit: have -> has

262

nit: this unreachable is unneeded because of the default case above

daltenty added inline comments.Oct 17 2019, 10:45 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
121

Should we specify this relative to XCOFF::N_DEBUG?

jasonliu updated this revision to Diff 225479.Oct 17 2019, 11:19 AM

Address David's comments.

jasonliu marked 3 inline comments as done.Oct 17 2019, 11:20 AM
sfertile added inline comments.Oct 18 2019, 10:27 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
83

Minor nit:
We have a 2 stage problem when deciding to use label defs or not:
Global: Does the type of csect allow/support labels defs.
Local: If label defs are supported in the csect type, can we omit the label defs for this specific csect.

I believe right now we are always emitting a label def for csects that support them even if it could be skipped, however there has been discussion to skip them when appropriate. UseLabelDefinition to me sounds like its saying implying yes to both the global and local criteria. In that light maybe enum values of 'LabelDefSupported' and 'LabelDefUnsupported' are more appropriate. (Or we can adjust the names when we implement the selective label def emission).

166–168

Minor nit: IIUC this is all the sections, as opposed to the non-empty sections. We can determine if a section is non-empty when it has been assigned a valid section index.

329

Should we maintain CurrentAddressLocation across groups to make sure there is no padding between groups?

573

I think we should consider using llvm::all_of to check if all the groups are empty and continue if they are. Then the section initialization can be done before processing any of the csects, without having to gate the initialization on if its section index is already valid , as well as adjust the size after processing the csects without an addition check to see if the section was initialized or not.

586

I think a fatal error would be more appropriate.

Minor nit: should we be creating a named constant for readability/documentation.

jasonliu updated this revision to Diff 225693.Oct 18 2019, 2:17 PM
jasonliu marked 8 inline comments as done.

Address Sean's comment.

llvm/lib/MC/XCOFFObjectWriter.cpp
83

Agree.

329

Do you mean maintain CurrentAddressLocation across Sections? Since it's currently already maintained across groups.
Made the changes based on my interpretation.

573

I still need a guard for
Section->Address = Group->Csects.front().Address;

But I do think using a specific guard for that is better than just comparing UninitializedIndex everywhere.
Thanks.

DiggerLin added inline comments.Oct 21 2019, 7:54 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
121

in the symbol table , the section index maybe has -2 ,-1 ,and 0. but for section, it always start from 1, I think keep original -1 as uninitiated is OK too.

573

two Csect.Size = Layout.getSectionAddressSize(MCSec); (duplicate with line 568 ) ?

jasonliu marked 2 inline comments as done.Oct 21 2019, 9:20 AM
jasonliu added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
121

It might be Okay depending on how we write our code later. But there is a chance we could run into risks of needing differentiate a special section number with a truely uninitialized section. So I would prefer to pick a section number that's not used to represent other things.

573

Thanks. I will address.

jasonliu updated this revision to Diff 225903.Oct 21 2019, 9:31 AM

Address Digger's comment.

jasonliu marked 2 inline comments as done.Oct 21 2019, 9:32 AM

@DiggerLin @daltenty @sfertile
Ping.
I believe I have addressed the comments you put.

This revision is now accepted and ready to land.Oct 23 2019, 7:55 AM
This revision was automatically updated to reflect the committed changes.