This is an archive of the discontinued LLVM Phabricator instance.

Implement -frecord-command-line for XCOFF integrated assembler path
ClosedPublic

Authored by Jake-Egan on Jul 10 2023, 10:45 PM.

Details

Summary

The patch D153600 implemented -frecord-command-line for the XCOFF direct assembly path. This patch adds support for the XCOFF integrated assembly path.

Diff Detail

Event Timeline

Jake-Egan created this revision.Jul 10 2023, 10:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2023, 10:45 PM
Jake-Egan requested review of this revision.Jul 10 2023, 10:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2023, 10:45 PM
Jake-Egan retitled this revision from Support -frecord-command-line on XCOFF integrated assembler path to Support -frecord-command-line for XCOFF integrated assembler path.Jul 10 2023, 10:50 PM
Jake-Egan edited the summary of this revision. (Show Details)
Jake-Egan updated this revision to Diff 539104.Jul 11 2023, 7:54 AM

Fix endianness of the padding.

Jake-Egan updated this revision to Diff 539106.Jul 11 2023, 7:56 AM

Remove old comment

Thank you for the patch!

Again, I'm not a COFF expert, so I only really reviewed the code itself, not how it implements the (X)COFF format

I think some of my comments are really just extensions of my confusion/disagreement with the style of the whole translation unit, but "leave things better than you found them" and all

llvm/include/llvm/MC/MCObjectWriter.h
114

In some places this is called Data, in others Metadata; if there is no underlying reason I'm missing can they all be switched to one or the other?

llvm/lib/MC/XCOFFObjectWriter.cpp
259–260

It seems like the StringRef member is redundant; every use can just be of the std::string version. Constructing transient StringRefs where needed doesn't seem expensive enough to warrant caching it in this way, as it adds code complexity and bloats the struct at runtime somewhat. If you are OK with the larger struct you could also consider caching the values for paddingSize and size directly instead.

If you just want to avoid the potentially costly/inconsistent implementation of std::string methods then SmallString<0> is a better alternative

280

formatting

628–631

The direct access to Entry here combined with the abstracted check for hasCInfoSymSection feels awkward

If the section supports multiple entries I would suggest just embracing that and making the interface be an iterator or a collection. It doesn't seem like more work to support, considering a SmallVector<..., 1> should already implement everything. If the extra size in the case where no entry is present is a concern then a SmallVector<..., 0> would also work.

If you would rather not support multiple entries because there is no use-case for it yet and you don't want to test for the support, then I'd suggest just dropping the hasCInfoSymSection check in favor of directly checking the Entry member. Adding the partial abstraction just makes the result more complicated

1240–1274

This code is repeated in nearly identical form for every derivation of SectionEntry here; can it be made into a method on SectionEntry that is called here for each? Some of this can go in an NFC patch preceding this one

1322–1332

Is there a rationale for not making this a constructor? It is nice to have the name of the fields, but it is more verbose and means at least one extra move is possible in C++17 where CInfoSymSection.Entry = std::make_unique... is guaranteed to be in-place constructed IIUC.

And it seems like setting the entry without updating the section's size is never meaningful, so I would also suggest adding a method called setEntry or similar. I'm curious why the general style in this file seems to be to use struct for everything, and so hide nothing.

Overall I think my complaints center around places where verbosity is spread around and there is no single point where invariants are maintained

1608

StringRef is intended to be used by-value almost everywhere, see the last sentence in https://www.llvm.org/docs/ProgrammersManual.html#the-stringref-class

If you do want the lvalue reference here, it should at least be const qualified

Jake-Egan updated this revision to Diff 540064.Jul 13 2023, 8:59 AM

Addressed comments

Jake-Egan marked 5 inline comments as done.Jul 13 2023, 9:05 AM
Jake-Egan added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
628–631

I went with checking the Entry member directly. Multiple entries can be supported if/when more use cases for the .info section is added.

1240–1274

I posted a patch for this here https://reviews.llvm.org/D155199

Jake-Egan marked an inline comment as done.Jul 13 2023, 9:08 AM
Jake-Egan added inline comments.Jul 13 2023, 9:11 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
280

Do you mean that CInfoSymSectionEntry(StringRef N, int32_t Flags) : SectionEntry(N, Flags){}; should be split into two lines? It seems git-clang-format wants it on one line.

Thank you for making the changes!

One small nit: I prefer using the implicit conversion to bool rather than explicitly checking for nullptr

llvm/lib/MC/XCOFFObjectWriter.cpp
280

Ah, my eyes just kind of glazed over the ;, which is what seems to be causing clang-format to delete the space between the closing parens and the {. Sorry for the confusion!

I didn't realize it was even legal in this case (i.e. a semicolon after a method/constructor definition, not just a declaration), but it seems to just be an optional part of the grammar.

I'd suggest deleting the ; and re-running clang-format, but I don't actually know that there is any policy on it

628–630
1057
1115
1241
1323
1340
1603
Jake-Egan updated this revision to Diff 540262.Jul 13 2023, 9:07 PM
Jake-Egan marked an inline comment as not done.

Update patch

Jake-Egan marked 8 inline comments as done.Jul 13 2023, 9:08 PM

I reviewed this code with respect to XCOFF and I don't see any issues. As intended, this code only supports a single C_INFO symbol in the object file.

scott.linder accepted this revision.Jul 17 2023, 11:35 AM

LGTM, thank you for the revisions!

This revision is now accepted and ready to land.Jul 17 2023, 11:35 AM
Jake-Egan updated this revision to Diff 541825.EditedJul 18 2023, 8:08 PM

Thanks for the reviews. Just made some small changes:

  • Changed the write order of the info section to match XCOFF convention
  • Use advanceFileOffset now that it has landed
Jake-Egan retitled this revision from Support -frecord-command-line for XCOFF integrated assembler path to Implement -frecord-command-line for XCOFF integrated assembler path.Jul 18 2023, 8:23 PM
scott.linder accepted this revision.Jul 19 2023, 11:01 AM
This revision was automatically updated to reflect the committed changes.
Jake-Egan marked an inline comment as done.