The patch D153600 implemented -frecord-command-line for the XCOFF direct assembly path. This patch adds support for the XCOFF integrated assembly path.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 | ||
237–238 | 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 | |
258 | formatting | |
604–607 | 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 | |
1215–1249 | 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 | |
1312–1322 | 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 | |
1602 | 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 |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
604–607 | I went with checking the Entry member directly. Multiple entries can be supported if/when more use cases for the .info section is added. | |
1215–1249 | I posted a patch for this here https://reviews.llvm.org/D155199 |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
258 | 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 | ||
---|---|---|
258 | 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 | |
604–606 | ||
1032 | ||
1090 | ||
1223 | ||
1313 | ||
1336 | ||
1597 |
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.
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
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?