This is an archive of the discontinued LLVM Phabricator instance.

[lib/ObjectYAML] - Simplify the code that handles Content/Size fields.
ClosedPublic

Authored by grimar on Oct 9 2020, 5:25 AM.

Details

Summary

This is a follow-up for D89039 patch, which adds a support for
Content/Size for all sections.

Assuming that all of sections have a support of these 2 fields,
we can simplify and generalize the code.

Depends on D89039

Diff Detail

Event Timeline

grimar created this revision.Oct 9 2020, 5:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2020, 5:25 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
grimar requested review of this revision.Oct 9 2020, 5:25 AM
MaskRay accepted this revision.Oct 9 2020, 8:23 AM

Looks great!

This revision is now accepted and ready to land.Oct 9 2020, 8:23 AM
jhenderson added inline comments.Oct 12 2020, 1:31 AM
llvm/lib/ObjectYAML/ELFEmitter.cpp
1303–1304

I can see why this is needed, but why wasn't it needed before, and what's the impact of adding it? Same applies in a few other places too.

grimar marked an inline comment as done.Oct 12 2020, 2:26 AM
grimar added inline comments.
llvm/lib/ObjectYAML/ELFEmitter.cpp
1303–1304

Previously when we had either Content or Size, the code on the left wrote some data and returned.
And we report an error when both Entries and Content/Size are used together much earlier, it is a normal situation.

After this patch we handle Content/Size for all types of sections in a single common place before writeSectionContent method is called.
But we still have to call writeSectionContent to set section fields (e.g sh_entsize/sh_link etc) and/or
handle section specific Entries (like here). Without this condition we just crash when trying to access an empty Entries in a loop below.

Perhaps worth to rename writeSectionContent to finalizeSection or alike.

jhenderson accepted this revision.Oct 13 2020, 12:52 AM

LGTM.

llvm/lib/ObjectYAML/ELFEmitter.cpp
1303–1304

Got it, thanks!

This revision was automatically updated to reflect the committed changes.
grimar marked an inline comment as done.