Page MenuHomePhabricator

[XCOFF][yaml2obj] support for the auxiliary file header.

Authored by Esme on Oct 9 2021, 4:12 AM.



This patch adds yaml2obj supporting for the auxiliary file header of XCOFF.

Diff Detail

Event Timeline

Esme created this revision.Oct 9 2021, 4:12 AM
Esme requested review of this revision.Oct 9 2021, 4:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2021, 4:12 AM
Esme updated this revision to Diff 378567.Oct 10 2021, 9:58 PM
jhenderson added inline comments.Oct 15 2021, 12:52 AM

I just remembered that llvm::Optional has a getValueOr(T Value) function which gives you the input parameter, if the optional is none, or the stored value otherwise. I think that would make the code throughout much simpler.


Do you actually need the casts? You've explicitly specified the type for write after all. If you do need a cast, use static_cast.


Give the 32768 magic number a name, i.e. put it in a variable or constant somewhere. Flags are also usually written as hex in the source code, which might help make things clearer.

8 ↗(On Diff #378567)

This and the 64-bit test need to show that the value isn't taken from the sections if present, i.e. they need to have at least one section of each type with values different to the explicitly specified values in the YAML.

1 ↗(On Diff #378567)

I think you need a 32-bit and 64-bit version of this test. In other words, show what the default is for both formats. Does that make sense to you?

I think you also need to show what happens when there are no text/data/bss sections etc from which to derive the values if not specified.

8 ↗(On Diff #378567)

I might be being dumb, but the sizes below look like 2 bytes to me.

47 ↗(On Diff #378567)

Assuming that a single section contributes to the field, you should use a different size here, to show that the right one is picked.

48–49 ↗(On Diff #378567)

Don't you need duplicates of this and some of the other sections, like .text, to show that the first one wins in all cases?

Esme updated this revision to Diff 380586.Oct 18 2021, 9:52 PM
Esme marked 7 inline comments as done.
Esme added inline comments.

I used the function getValueOr(T Value) in XCOFFWriter::writeAuxFileHeader(), which make the code much simpler.
While I didn't use it here because I think it would be better to use the if condition to bail out early rather than call getValueOr() every time. As well as it will not make the code simpler here.


The casts were necessary in the comparison expressions.
However I don't need them anymore because the getValueOr(T Value) is adopted now.

1 ↗(On Diff #378567)

Yes! The suggestions make sense to me. Thanks!

8 ↗(On Diff #378567)

Because the DefaultSectionAlign is 4 in XCOFF.
If I didn't set the sizes explicitly, the calculated values are used.

DiggerLin added inline comments.Oct 29 2021, 9:57 AM

should be yaml::Hex8(0) for flag.

69 ↗(On Diff #380586)

in the xcoff document , some section are not multi section allow.
for example, loader section. but it is a test , it do not matter.

DiggerLin accepted this revision.Oct 29 2021, 10:12 AM

there is some nit comment need to be addressed.

This revision is now accepted and ready to land.Oct 29 2021, 10:12 AM
jhenderson added inline comments.Mon, Nov 1, 8:39 AM

I might put the comment on the same line, because it's a little confusing whether it's referring to just this one field or not.


Just checking: should this comment be CPU rather than CUP?


These numbers aren't obviously arbitrary, I'd suggest making them more obviously so, e.g. 0x1111, 0x2222 etc.

1 ↗(On Diff #380586)

Perhaps rename the test to aux-hdr-defaults.yaml.

50 ↗(On Diff #380586)

Probably fill with non-zero values all the way. Maybe also add a comment why you have two of each section.

69 ↗(On Diff #380586)

Remember, this is yaml2obj, so we should be as flexible as possible.

74 ↗(On Diff #380586)

Same below.

I wonder if you could reuse most of the FileCheck data here with the 32-bit case, rather than duplicating it? I've not looked too closely, but perhaps multiple check-prefixes would be appropriate, e.g. --check-prefixes=COMMON1,CASE1-64?

113 ↗(On Diff #380586)
8 ↗(On Diff #378567)

So in XCOFF are sections tail padded? In ELF, there are just unlabelled gaps between sections, if alignment padding is required.

DiggerLin added inline comments.Mon, Nov 1, 10:58 AM
74 ↗(On Diff #380586)

the data members and the order the data members of auxiliary header between 32bit and 64bit are different , if using COMMON1,CASE1-64 , I think it maybe more difficult to read the test case.

Esme updated this revision to Diff 385389.Sun, Nov 7, 6:43 PM
Esme marked an inline comment as done.

Address comments.

50 ↗(On Diff #380586)

I think setting section content with non-zero values is enough since the size and address will be derived automatically.

74 ↗(On Diff #380586)

Thanks for your comments and agree with Digger to keep the current check-style.

8 ↗(On Diff #378567)

Yes, it's required that the section address aligned to DefaultSectionAlign.

jhenderson accepted this revision.Mon, Nov 8, 12:43 AM

Two minor comments, otherwise LGTM.


Could we add something before .text so that the .text section start address is non-zero? (This may not make sense from a file format perspective, so fine if not)

Esme marked an inline comment as done.Tue, Nov 9, 1:49 AM
Esme added inline comments.

Yes, it sounds reasonable to me.

This revision was landed with ongoing or failed builds.Tue, Nov 9, 1:50 AM
This revision was automatically updated to reflect the committed changes.
RKSimon added a subscriber: RKSimon.Tue, Nov 9, 2:31 AM
RKSimon added inline comments.

This is failing on MSVC builds:

E:\llvm\llvm-project\llvm\include\llvm/ADT/Optional.h(298): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
E:\llvm\llvm-project\llvm\lib\ObjectYAML\XCOFFEmitter.cpp(356): note: see reference to function template instantiation 'T llvm::Optional<T>::getValueOr<llvm::yaml::Hex32>(U &&) const &' being compiled