Page MenuHomePhabricator

[yaml2obj][XCOFF] add the SectionIndex field for symbol.
ClosedPublic

Authored by Esme on Sep 9 2021, 10:23 PM.

Details

Summary

This is found during testing the error paths in D98003.
See the error case 2 in llvm/test/tools/obj2yaml/XCOFF/invalid.yaml of D98003.

Diff Detail

Event Timeline

Esme created this revision.Sep 9 2021, 10:23 PM
Esme requested review of this revision.Sep 9 2021, 10:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2021, 10:23 PM

Hmm, I think maybe this should be a yaml2obj error rather than just writing some arbitrary value. If a symbol references a non-existent section by name, it's just dodgy YAML. (OTOH, it would be perfectly reasonable to allow specification by index and for an invalid index to be allowed without error).

Esme updated this revision to Diff 372108.Sep 12 2021, 12:38 AM
Esme retitled this revision from [yaml2obj][XCOFF] write an invalid index for an invalid Section name specified in Symbol to [yaml2obj][XCOFF] add the SectionIndex field for symbol..

Add the SectionIndex field for symbol.
1: a symbol can reference a section by SectionName or SectionIndex.
2: a symbol can reference a section by both SectionName and SectionIndex.
3: if both Section and SectionIndex are specified, but the two values refer to different sections, an error will be reported.
4: an invalid SectionIndex is allowed.
5: if a symbol references a non-existent section by SectionName, an error will be reported.

Higuoxing added inline comments.Sep 12 2021, 8:42 AM
llvm/lib/ObjectYAML/XCOFFEmitter.cpp
391
llvm/test/tools/yaml2obj/XCOFF/symbol-section.yaml
74

It might be good to report the invalid section index in the error message. e.g.,

the SectionIndex 2 specified in the symbol is invalid.
Esme updated this revision to Diff 372155.Sep 12 2021, 9:16 PM

Address comment

Some small points, but otherwise looks pretty good to me.

llvm/lib/ObjectYAML/XCOFFEmitter.cpp
393

Perhaps "does not exist" instead of "is invalid"? Up to you though.

398

You should specify the name and index in the error message, as otherwise a user won't be able to see which symbol is causing the problem.

llvm/test/tools/yaml2obj/XCOFF/symbol-section.yaml
38
74

Aside: this error message from llvm-readobj should ideally be updated to include the symbol name or index, so that a user can see which symbol is the problem. Fix in a different patch though.

Esme updated this revision to Diff 372184.Sep 13 2021, 2:20 AM

Thanks!
Address comment.

llvm/test/tools/yaml2obj/XCOFF/symbol-section.yaml
74

Leave a TODO in this patch.

jhenderson accepted this revision.Sep 13 2021, 3:34 AM

LGTM; please give @Higuoxing a chance to comment again.

This revision is now accepted and ready to land.Sep 13 2021, 3:34 AM
Higuoxing accepted this revision.Sep 13 2021, 5:30 AM

LGTM, thanks!

This revision was landed with ongoing or failed builds.Sep 13 2021, 11:19 PM
This revision was automatically updated to reflect the committed changes.