This is an archive of the discontinued LLVM Phabricator instance.

[NFC][XCOFF] Refactor and format XCOFFObjectWriter.cpp.
ClosedPublic

Authored by Esme on Mar 2 2022, 5:34 PM.

Diff Detail

Event Timeline

Esme created this revision.Mar 2 2022, 5:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 5:34 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
Esme requested review of this revision.Mar 2 2022, 5:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 5:34 PM
Esme updated this revision to Diff 412579.Mar 2 2022, 5:39 PM
DiggerLin added inline comments.Mar 4 2022, 12:47 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
566–567

do we still need is64Bit() ?

567–569

" in 32-bit mode" will be deleted ?

662–669

change writeSymbolBody to writeSymbolEntry ?

664

do we want to set a default AuxCnt=1 ?

676–677

delete the comment.

680

change Name to writeSymbolAuxCsectEntry ?

682

I am not sure which ( "x_scnlen" or "SectionOrLength")one is better for comment ? maybe the SectionOrLength from XCOFFObjectFile.h is better.

691

function name change to writeSymbolAuxDwarfEntry ?

743–744

since the header is for 32bit.

W.write<uint32_t>(SymbolTableOffset);    // f_symptr
W.write<int32_t>(SymbolTableEntryCount); // f_nsyms

we should modify the code
as
assert(!is64Bit() && "not support 64 bit for writing file header.");
W.write<uint16_t>(XCOFF::XCOFF32);

769–770

I do not think we need the s_size here , Sec->Size can self-explain.
ditto for the following.

DiggerLin added inline comments.Mar 4 2022, 12:51 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
823–829

we can use ".file" directly instead of StringRef(".file")

Esme updated this revision to Diff 413335.Mar 6 2022, 9:17 PM
Esme marked 8 inline comments as done.

Addressed @DiggerLin 's comments. Thanks

llvm/lib/MC/XCOFFObjectWriter.cpp
682

I am not sure which ( "x_scnlen" or "SectionOrLength")one is better for comment ? maybe the SectionOrLength from XCOFFObjectFile.h is better.

682

I would prefer the x_scnlen one because it is easier for the developer to refer to the documentation, and this comment style references ELFObjectWriter.cpp.

743–744

Thanks!
Well, I don't think we need assertions here, because we already blocked 64-bit mode with another assertion before calling this function.

769–770

Removed // s_size and // s_flags, and kept other comments.

DiggerLin added inline comments.Mar 7 2022, 10:32 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
201

change to
constexpr uint64_t MaxRawDataSize =

DiggerLin added inline comments.Mar 7 2022, 10:35 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
201

sorry should be change to const uint64_t MaxRawDataSize =

DiggerLin added inline comments.Mar 7 2022, 1:06 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
278

using writeSymbolAuxCsectEntry(uint64_t SecLen, uint8_t Type, uint8_t MapClass) is easy to understand the interface than writeSymbolAuxCsectEntry(uint64_t, uint8_t, uint8_t);
same as
writeSymbolAuxDwarfEntry
writeSymbolEntry

662–669

uint64_t Value--> uint32_t Value

665

maybe better to change the AuxCnt to NumberOfAuxEntries ?
change SecIdx to SectionNumber
etc ,
I think the parameter name should be consistent with XCOFFObject.h.

same as function
writeSymbolAuxDwarfEntry and writeSymbolAuxCsectEntry

682

the function looks only support 32bit AuxCsect now . "uint64_t SecLen" should be "uint32_t SecLen"? if we change to "uint32_t SecLen"

W.write<uint32_t>(Lo_32(SecLen)); // x_scnlen

also need to be changed.

693

"uint64_t SecLen" should be "uint32_t SecLen" ? and "uint64_t RelCnt" should be "uint32_t RelCnt"

720–723

change "DwarfSectionRef.getSymbolTableName(), 0, SectionIndex, 0,"
-->
DwarfSectionRef.getSymbolTableName(), /*Value=*/0, SectionIndex,/*Type=*/ 0,

adding comment will make the value "0" easy to understand.

730

add a comment for value 0, ditto in the following value zero or 1

jhenderson added inline comments.Mar 8 2022, 12:04 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
276–279

Please provide the parameter names here.

694

Default arguments are usually specified in the declaration, not definition. Same elsewhere.

701

For consistency with other names.

743–744

I'd have the assertion: in the future, you may not call this function from the same place.

822–823

Ditto the line below.

Esme updated this revision to Diff 414002.Mar 8 2022, 9:14 PM
Esme marked 13 inline comments as done.

Addressed @DiggerLin and @jhenderson 's comments. Thanks!

Esme updated this revision to Diff 414004.Mar 8 2022, 9:41 PM

Updated comments for consistency with parameters' names.

jhenderson added inline comments.Mar 8 2022, 11:10 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
710

Here and similar comments elsewhere, the format is as suggested inline.

DiggerLin added inline comments.Mar 9 2022, 8:22 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
667

the writeSymbolName(SymbolName) can self explain .
the comment "Magic, Offset, SymbolName " description of functionality of the writeSymbolName(), if you want to put the comment, it maybe better to put before the function writeSymbolName definition.

Esme updated this revision to Diff 414576.Mar 10 2022, 9:54 PM

Addressed comments.

jhenderson accepted this revision.Mar 10 2022, 10:37 PM

LGTM, but please wait for @DiggerLin

This revision is now accepted and ready to land.Mar 10 2022, 10:37 PM
DiggerLin accepted this revision.Mar 14 2022, 1:52 PM

LGTM , but need to fix two comments.

llvm/lib/MC/XCOFFObjectWriter.cpp
650

Nit: // Magic, Offset or SymbolName.

827

writeSymbolEntry(".file", /*Type=*/0, XCOFF::ReservedSectionNum::N_DEBUG,

->
writeSymbolEntry(".file", /*Value=*/0, XCOFF::ReservedSectionNum::N_DEBUG,

This revision was landed with ongoing or failed builds.Mar 14 2022, 11:41 PM
This revision was automatically updated to reflect the committed changes.