The patch adds support for yaml2obj parsing auxiliary symbols for XCOFF.
Details
- Reviewers
qiucf shchenz jhenderson Higuoxing jsji DiggerLin - Group Reviewers
Restricted Project - Commits
- rG817936408bad: [yaml2obj][XCOFF] parsing auxiliary symbols.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
llvm/include/llvm/ObjectYAML/XCOFFYAML.h | ||
---|---|---|
197 | 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 | ||
209 | Nit. | |
211 | ||
242–243 | Nit: coding style. | |
244 | It would be great to print the values of NumberOfAuxEntries and AuxCount in the error message. | |
llvm/lib/ObjectYAML/XCOFFYAML.cpp | ||
252–253 | Why the names of these two entries are not consistent with other fields (e.g., SectionOrLengthLo, SectionOrLengthHi)? |
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
---|---|---|
701–717 | It would be great to move these if-else statements into a helper function. |
llvm/include/llvm/ObjectYAML/XCOFFYAML.h | ||
---|---|---|
150 | I'm not sure that these comments are really useful. I'd just get rid of them. | |
llvm/lib/ObjectYAML/XCOFFYAML.cpp | ||
294–295 | 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. |
Address comments.
llvm/include/llvm/ObjectYAML/XCOFFYAML.h | ||
---|---|---|
197 | 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 | ||
294–295 | I think this should be an error.
| |
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. |
llvm/tools/obj2yaml/xcoff2yaml.cpp | ||
136–142 | Agree, thanks. |
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
---|---|---|
241 | You could store this a uint32_t to save the later cast, if you want. | |
244 | 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. |
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.
Apologies, no idea how this one slipped off the radar. Mostly looks good, but some test comments.
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
---|---|---|
643 | I don't think you've got a test case for this case? | |
704–705 | Is there testing for this case? | |
llvm/test/tools/yaml2obj/XCOFF/aux-symbols.yaml | ||
2 ↗ | (On Diff #390612) | |
456 ↗ | (On Diff #390612) | |
496–501 ↗ | (On Diff #390612) | 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 ↗ | (On Diff #390612) | Same sort of comment as above. |
527 ↗ | (On Diff #390612) | Nit: double blank line |
529–530 ↗ | (On Diff #390612) |
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 :-)
llvm/include/llvm/ObjectYAML/XCOFFYAML.h | ||
---|---|---|
93 | there is enum define as enum SymbolAuxType in xcoff.h, I think you can use it instead of define a new one. | |
96 | please add some comment here " it is for 64bits only" | |
121 | 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. | |
142 | ExcpetionAuxEnt is 64bit only and OffsetToExceptionTbl is 8 bytes. | |
197 | 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.
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 | ||
---|---|---|
93 | 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. | |
96 | 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. | |
121 | Nice comments, thanks. | |
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
643 | 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. |
LGTM, except for one nit, but please also make sure @DiggerLin is happy.
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
---|---|---|
643 | If it's unreachable, why not use llvm_unreachable? |
there is enum define as enum SymbolAuxType in xcoff.h, I think you can use it instead of define a new one.