[PATCH] D153600: Implement -frecord-command-line for XCOFF

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

Sorry for the delay in review, I am not too familiar with XCOFF so I was hoping someone else would take a look first.

If my understanding of the COFF docs I could find is correct then this LGTM save for some nits/suggestions



================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:970-1037
+void MCAsmStreamer::emitXCOFFCInfoSym(StringRef Name, StringRef Metadata) {
+  const char *InfoDirective = "\t.info ";
+
+  // Start by emitting the .info pseudo-op and C_INFO symbol name
+  OS << InfoDirective;
+  PrintQuotedString(Name, OS);
+  OS << ", ";
----------------
I sketched out some of the suggestions I had, although the bigger changes are just because the extra `.info` for the padded byte bugged me. If you aren't concerned with it I'm also happy with what you have


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:980
+  size_t MetadataSize = Metadata.size();
+  uint32_t MetadataPaddingSize = 3 - (MetadataSize - 1) % 4;
+
----------------
I couldn't quickly find a reference for the alignment requirement, but it seems like there is an additional requirement that the length must also be non-zero, not just even?

If so, can you update the comment?

I would also rather explicitly use `alignTo` and `max` to express this (see suggestion), but if we don't expect the optimizer to clean it up I'm fine with the more terse version.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:980
+  size_t MetadataSize = Metadata.size();
+  uint32_t MetadataPaddingSize = 3 - (MetadataSize - 1) % 4;
+
----------------
scott.linder wrote:
> I couldn't quickly find a reference for the alignment requirement, but it seems like there is an additional requirement that the length must also be non-zero, not just even?
> 
> If so, can you update the comment?
> 
> I would also rather explicitly use `alignTo` and `max` to express this (see suggestion), but if we don't expect the optimizer to clean it up I'm fine with the more terse version.
Can you factor this out at function scope? It gets repeated below


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:984
+  uint32_t Length = MetadataSize + MetadataPaddingSize;
+  OS << format_hex(uint32_t(Length), 10) << ",";
+  EmitEOL();
----------------
Redundant cast


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:988
+  // Return the remaining bytes padded with 0s.
+  auto GetLastWord = [](const uint8_t *Data,
+                        uint32_t PaddingBytes) -> uint32_t {
----------------
It seems odd to use a lambda when this is only used once. Why not just compute the last word directly?


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1033
+                                    MetadataPaddingSize);
+    OS << InfoDirective << ", ";
+    OS << format_hex(LastWord, 10);
----------------
Can you factor this out as `Separator` at function scope?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-command-line-metadata.ll:21
+; Trailing padding:
+; ASM: .info , 0x32330000
+
----------------
Having the padded byte force a new `.info` directive doesn't seem ideal. I suggested something to avoid it, but I suppose it also doesn't really harm anything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153600