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

[Jake Egan via Phabricator via llvm-commits]

Jake-Egan marked 5 inline comments as done.
Jake-Egan added inline comments.


================
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);
+
----------------
scott.linder wrote:
> 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
I went with checking the Entry member directly. Multiple entries can be supported if/when more use cases for the .info section is added.


================
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.");
+  }
+
----------------
scott.linder wrote:
> 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
I posted a patch for this here https://reviews.llvm.org/D155199


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154921