Page MenuHomePhabricator

[yaml2obj][XCOFF] parsing auxiliary symbols.
Needs ReviewPublic

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

Details

Reviewers
qiucf
shchenz
jhenderson
Higuoxing
Group Reviewers
Restricted Project
Summary

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

Diff Detail

Event Timeline

Esme created this revision.Wed, Nov 10, 2:17 AM
Esme requested review of this revision.Wed, Nov 10, 2:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Nov 10, 2:17 AM
Esme edited the summary of this revision. (Show Details)Wed, Nov 10, 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.Wed, Nov 10, 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.Wed, Nov 10, 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.Thu, Nov 18, 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.Thu, Nov 25, 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.Fri, Nov 26, 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.Tue, Nov 30, 1:20 AM

Address comments.