[PATCH] D154921: Support -frecord-command-line for XCOFF integrated assembler path

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a 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



================
Comment at: llvm/include/llvm/MC/MCObjectWriter.h:114
+
+  virtual void addCInfoSymEntry(StringRef Name, StringRef Data) {
+    report_fatal_error("addCInfoSymEntry is only supported on XCOFF targets");
----------------
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?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:237-238
+struct CInfoSymInfo {
+  std::string MetadataStr;
+  StringRef Metadata;
+  // Name of the C_INFO symbol associated with the section
----------------
It seems like the `StringRef` member is redundant; every use can just be of the `std::string` version. Constructing transient `StringRef`s 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


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:258
+
+  CInfoSymSectionEntry(StringRef N, int32_t Flags) : SectionEntry(N, Flags){};
+  virtual ~CInfoSymSectionEntry() = default;
----------------
formatting


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:604-607
+  std::unique_ptr<CInfoSymInfo> &CISI = CInfoSymSection.Entry;
+  if (hasCInfoSymSection() && nameShouldBeInStringTable(CISI->Name))
+    Strings.add(CISI->Name);
+
----------------
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


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1215-1249
+  if (hasCInfoSymSection()) {
+    CInfoSymSection.FileOffsetToData = RawPointer;
+    RawPointer += CInfoSymSection.Size;
+    assert(RawPointer <= MaxRawDataSize &&
+           "Section raw data overflowed this object file.");
+  }
+
----------------
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


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1312-1322
+void XCOFFObjectWriter::addCInfoSymEntry(StringRef Name, StringRef Data) {
+  std::unique_ptr<CInfoSymInfo> NewEntry = std::make_unique<CInfoSymInfo>();
+  NewEntry->MetadataStr = Data.str();
+  NewEntry->Metadata = StringRef(NewEntry->MetadataStr);
+  NewEntry->Name = Name.str();
+  NewEntry->Offset = sizeof(uint32_t);
+  CInfoSymSection.Size += NewEntry->size();
----------------
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


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1602
+  std::unique_ptr<CInfoSymInfo> &CISI = CInfoSymEntry.Entry;
+  StringRef &Metadata = CISI->Metadata;
+
----------------
`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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154921/new/

https://reviews.llvm.org/D154921