This is an archive of the discontinued LLVM Phabricator instance.

[Object][NFC] Factor out computeHeadersSize.
ClosedPublic

Authored by jacek on Feb 7 2023, 4:41 PM.

Details

Summary

In preparation for COFF archives support.

Diff Detail

Unit TestsFailed

Event Timeline

jacek created this revision.Feb 7 2023, 4:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2023, 4:41 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
jacek requested review of this revision.Feb 7 2023, 4:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2023, 4:41 PM
jacek retitled this revision from [Object 2/5] Factor out computeHeadersSize. to [Object][NFC] Factor out computeHeadersSize..Feb 8 2023, 12:47 PM
mstorsjo added a subscriber: mstorsjo.

I didn't follow the actual refactoring here yet (it seems at least somewhat nontrivial) - but if this is indeed a pure refactoring, add NFC ("no functional change") as marker to the end of the commit message; in principle, commits are either NFC (with no changes to tests), or if there's a functional change, it should also have a matching test update.

I didn't follow the actual refactoring here yet (it seems at least somewhat nontrivial) - but if this is indeed a pure refactoring, add NFC ("no functional change") as marker to the end of the commit message; in principle, commits are either NFC (with no changes to tests), or if there's a functional change, it should also have a matching test update.

Oh, sorry, I somehow entirely missed that this patch already had the NFC tag. Sorry for the noise...

The computeSymbolTableSize change is reasonable. Other changes increase the number of lines. I do not look at follow-ups so cannot tell whether this is reasonable.

llvm/lib/Object/ArchiveWriter.cpp
416

return strlen("!<arch>\n") + computeSymbolTableHeaderSize() + SymtabSize;

The compiler will optimize out strlen. Avoid trivially-used-once variables.

jacek updated this revision to Diff 496114.Feb 9 2023, 7:00 AM
jacek added a comment.Feb 9 2023, 7:09 AM

The computeSymbolTableSize change is reasonable. Other changes increase the number of lines. I do not look at follow-ups so cannot tell whether this is reasonable.

I updated the diff with suggested change. I need separated HeaderSize in later patches to avoid calling computeSymbolTableHeaderSize multiple times, but I moved adding variables later in the series.

Other changes are mostly meant to pass objects' offsets to writeSymbolTable instead of computing it there from scratch. That computation is trivial now, but it becomes more tricky with later patches, as it would need to take second symbol table, string table and EC symbols table into account (both in writeSymbolTable and in new functions writing additional tables).

llvm/lib/Object/ArchiveWriter.cpp
416

I may change it here, but then I'd need to introduce them in later patches. HeaderSize is useful, because it avoids calling computeSymbolTableHeaderSize up to 3 times, when we have another headers for the second symbol table and EC symbols.

efriedma added inline comments.Mar 21 2023, 1:03 PM
llvm/lib/Object/ArchiveWriter.cpp
700

It's not obvious to me what this constant 8 was supposed to mean, or why it's changing to zero.

711

Please use Optional<uint64_t> to clarify the intent here.

jacek added inline comments.Mar 22 2023, 8:36 AM
llvm/lib/Object/ArchiveWriter.cpp
700

8 meant strlen("!<arch>\n"), I changed it to 0 because computeHeadersSize now takes care of it. I will change bug archive code path to be more similar to other formats, I think it will make it easier to follow.

jacek updated this revision to Diff 507374.Mar 22 2023, 8:52 AM

Thanks for review. The new version uses llvm::Optional and changes big archive code path to be slightly more similar to other formats.

FYI, most (all?) use of llvm::Optional has been migrated to std::optional these days.

FYI, most (all?) use of llvm::Optional has been migrated to std::optional these days.

Thanks, I wasn't sure which one I should use. I will update the patch.

jacek updated this revision to Diff 507423.Mar 22 2023, 10:24 AM

Using std::optional instead of llvm::Optional.

This revision is now accepted and ready to land.Mar 22 2023, 12:30 PM
This revision was landed with ongoing or failed builds.Mar 23 2023, 4:44 AM
This revision was automatically updated to reflect the committed changes.