This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj][XCOFF] parsing auxiliary symbols.
ClosedPublic

Authored by Esme on Nov 10 2021, 2:17 AM.

Details

Summary

The patch adds support for yaml2obj parsing auxiliary symbols for XCOFF.

Diff Detail

Event Timeline

Esme created this revision.Nov 10 2021, 2:17 AM
Esme requested review of this revision.Nov 10 2021, 2:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2021, 2:17 AM
Esme edited the summary of this revision. (Show Details)Nov 10 2021, 2:18 AM

I'd recommend including DiggerLin in the subscriber lists for these, since he is using yaml2obj for XCOFF quite a bit. I'll get to this review in the next couple of days hopefully.

Higuoxing added inline comments.Nov 10 2021, 6:52 PM
llvm/include/llvm/ObjectYAML/XCOFFYAML.h
160

Question 1: Why we need std::unique_ptr<> here?

Question 2: It looks that AuxEntries is optional in the symbol entry. Can we wrap it with Optional<>.

llvm/lib/ObjectYAML/XCOFFEmitter.cpp
206

Nit.

208
239–240

Nit: coding style.

241

It would be great to print the values of NumberOfAuxEntries and AuxCount in the error message.

llvm/lib/ObjectYAML/XCOFFYAML.cpp
221–222

Why the names of these two entries are not consistent with other fields (e.g., SectionOrLengthLo, SectionOrLengthHi)?

Higuoxing added inline comments.Nov 10 2021, 6:59 PM
llvm/lib/ObjectYAML/XCOFFEmitter.cpp
574–590

It would be great to move these if-else statements into a helper function.

jhenderson added inline comments.Nov 18 2021, 12:52 AM
llvm/include/llvm/ObjectYAML/XCOFFYAML.h
118

I'm not sure that these comments are really useful. I'd just get rid of them.

llvm/lib/ObjectYAML/XCOFFYAML.cpp
263–264

Should this really be an error? Could you potentially have an AUX_EXCEPT symbol in a 32-bit object (albeit a malformed one)? Or is it fundamentally impossible to write such a thing? Same goes for the AUX_STAT symbol below.

Remember, in yaml2obj, we want to be as permissive as possible. If you could imagine something existing, it should be allowed, even if the file format techincally prohibits it. This then allows you to write testing for code that reads the file (e.g. llvm-readobj) to ensure it properly handles such cases.

llvm/test/tools/yaml2obj/XCOFF/aux-symbols-defaults.yaml
3 ↗(On Diff #386099)

Rather than have one test-case per aux entry type, is it possible to have one which contains all entry types, either with a single symbol, or multiple symbols, as needed?

In the mulitple symbol case, name the symbols after their aux entry.

38 ↗(On Diff #386099)
llvm/test/tools/yaml2obj/XCOFF/aux-symbols-full-contents.yaml
1 ↗(On Diff #386099)

Rather than needing to duplicate the YAML between this and the defaults test case, you could consider implementing support for (if needed) and using the special [[MACRO=<none>]] syntax in the YAML. For ELF, this is equivalent to omitting the field, if no value is specified for it, which allows you to test default behaviour, using the same YAML as non-default behaviour.

llvm/tools/obj2yaml/xcoff2yaml.cpp
136–142

Maybe spin this off into another patch? Seems unrelated to getting yaml2obj to actually produce the aux symbols, and needs its own testing.

Esme updated this revision to Diff 389703.Nov 25 2021, 2:28 AM
Esme marked 8 inline comments as done.

Address comments.

llvm/include/llvm/ObjectYAML/XCOFFYAML.h
160

I think there is no harm in using std::unique_ptr<> since it has less overhead. While wrapping it with Optional<> will cause failure in mapOptional().

llvm/lib/ObjectYAML/XCOFFYAML.cpp
263–264

I think this should be an error.

  1. When parsing a 32-bit object, the AuxSymbol type is determined by the Value and StorageClass of the symbol to which it belongs. Even if we intentionally write a case of AUX_EXCEPT symbol in XCOFF32, it cannot be parsed as AUX_EXCEPT, but as some other types, and it makes sense to parse it this way.
  2. As for the 64-bit object, the AuxSymbol type can also be recognized by the AuxSymbolType field, but the type XCOFF::AUX_STAT does not exist, so it is fundamentally impossible to write such a thing.
llvm/test/tools/yaml2obj/XCOFF/aux-symbols-defaults.yaml
3 ↗(On Diff #386099)

Thanks!

llvm/test/tools/yaml2obj/XCOFF/aux-symbols-full-contents.yaml
1 ↗(On Diff #386099)

The [[MACRO=<none>]] syntax will work well to reduce duplication when the same field is used for multiple assignments. But in this test, most of the fields only need to be tested once.
Besides, 32-bit and 64-bit may have different fields for the same aux symbol type, so I have to use two YAMLs. I don't think using [[MACRO=<none>]] here will simplify the test.
And I think it would be easier to review if I separate the default and non-default behaviors?

llvm/tools/obj2yaml/xcoff2yaml.cpp
136–142

Agree, thanks.

jhenderson added inline comments.Nov 26 2021, 12:43 AM
llvm/lib/ObjectYAML/XCOFFEmitter.cpp
238

You could store this a uint32_t to save the later cast, if you want.

241

I think we prefer static_cast

llvm/test/tools/yaml2obj/XCOFF/aux-symbols-full-contents.yaml
1 ↗(On Diff #386099)

You've got two general test cases: the default and the non default. In both cases, you need a fair amount of basic scaffolding for the YAML to work (i.e. symbol details etc). In the default case, you can omit some fields, for the aux entries, but not in the full contents cases. I'm suggesting you merge the default and non-default YAML documents, not the 32/64 cases, so that the basic scaffolding isn't duplicated across the two files.

I don't think it's any easier to review separating default and non-default cases, personally. You'll see this pattern a lot in the ELF tests.

Esme updated this revision to Diff 390612.Nov 30 2021, 1:20 AM

Address comments.

Esme added a comment.Dec 15 2021, 2:29 AM

Thank you so much for your reviewing. @jhenderson @Higuoxing
Do you have more comments for this patch?
Btw. there are 2 accepted patches (D113825 and D114434) depends on this patch.

Esme added a reviewer: jsji.Dec 15 2021, 2:29 AM

Apologies, no idea how this one slipped off the radar. Mostly looks good, but some test comments.

llvm/lib/ObjectYAML/XCOFFEmitter.cpp
516

I don't think you've got a test case for this case?

577–578

Is there testing for this case?

llvm/test/tools/yaml2obj/XCOFF/aux-symbols.yaml
2
456
496–501

This comment and error don't match up. According to the comment, I'd expect to see a complaint about using AUX_STAT in XCOFF32, but the error is soemthing about C_STAT symbols being defined in XCOFF32.

512–517

Same sort of comment as above.

527

Nit: double blank line

529–530

Thank you so much for your reviewing. @jhenderson @Higuoxing
Do you have more comments for this patch?
Btw. there are 2 accepted patches (D113825 and D114434) depends on this patch.

Sorry for not responding to this thread for a long time. I'm busy with my works these days. You can land this patch once @jhenderson is happy with it :-)

DiggerLin added inline comments.Dec 16 2021, 8:46 AM
llvm/include/llvm/ObjectYAML/XCOFFYAML.h
61

there is enum define as enum SymbolAuxType in xcoff.h, I think you can use it instead of define a new one.

64

please add some comment here " it is for 64bits only"

89

I am prefer to add some comments on the data member to indicate the one is for 32bits or 64bits only, or it is for both.
and rearrange the member in the order of common field for both 32bits and 64bits first , only for 32bits, only for the 64bits.

110

ExcpetionAuxEnt is 64bit only and OffsetToExceptionTbl is 8 bytes.

160

I think it need std::unique_ptr<> here, for the AuxEntries is std::vector<> of the pointer to AuxSymbolEnt, without the std::unique_ptr<>, we have to delete all the pointer of AuxSymbolEnt manually otherwise there is memory leak.

the x_auxtype is only for 64bits. in the implement, it means we always need to specific the AUX_type no matter it is 32bit or 64bits.

Esme updated this revision to Diff 395374.Dec 19 2021, 10:10 PM
Esme marked an inline comment as done.

Thanks! @jhenderson @DiggerLin @Higuoxing
Address comments.

Esme added a comment.EditedDec 19 2021, 10:12 PM

the x_auxtype is only for 64bits. in the implement, it means we always need to specific the AUX_type no matter it is 32bit or 64bits.

Yes, I think it's convenient for users to define an aux symbol with a specified type and it's straightforward for the designing of yaml2obj behaviors.

llvm/include/llvm/ObjectYAML/XCOFFYAML.h
61

XCOFF::SymbolAuxType is only for XCOFF64, therefor the AUX_STAT for XCOFF32 is not a member of XCOFF::SymbolAuxType. But it's required in yaml2obj (as what I have designed) when writing the section auxiliary entry for the C_STAT symbol.

64

It's required to determine the aux type when reading the yaml files, even we are not going to write the _auxtype field for XCOFF32. So it's not for 64bits only here.

89

Nice comments, thanks.

llvm/lib/ObjectYAML/XCOFFEmitter.cpp
516

In fact, this is a llvm_unreachable case because the XCOFFYAML::AuxSymbolType AuxType is a required field in Yaml input and a case with unknown auxiliary symbol type can't be generated in Yaml.

Esme updated this revision to Diff 395375.Dec 19 2021, 10:16 PM
Esme added a comment.Dec 28 2021, 10:10 PM

Gently ping.

jhenderson accepted this revision.Jan 5 2022, 6:24 AM

LGTM, except for one nit, but please also make sure @DiggerLin is happy.

llvm/lib/ObjectYAML/XCOFFEmitter.cpp
516

If it's unreachable, why not use llvm_unreachable?

This revision is now accepted and ready to land.Jan 5 2022, 6:24 AM
This revision was landed with ongoing or failed builds.Jan 9 2022, 6:39 PM
This revision was automatically updated to reflect the committed changes.