[PATCH] D106380: [SystemZ][z/OS] Initial code to generate assembly files on z/OS

[Tomas Matheson via Phabricator via llvm-commits]

tmatheson added inline comments.


================
Comment at: llvm/include/llvm/MC/MCSectionGOFF.h:37
+                            const MCExpr *Subsection) const override {
+    OS << "\t.section\t\"" << getName() << "\"\n";
+  }
----------------
anirudhp wrote:
> anirudhp wrote:
> > tmatheson wrote:
> > > It might be a good idea to generalise `printName` from `MCSectionELF.cpp` to handle outputting names in quotes with proper escaping.
> > Sure. Done.
> Sorry I just realized I didn't quite answer your question properly. Yes, I will port over the complete `printName` implementation from the ELF part over. 
> 
> However, when looking at it, I realized there;s a bug with this patch. The GOFF sections are printed, but the ELF section are printed too, and this doesn't quite make sense. There shouldn't be mixing of sections. So I will try to resolve that first.
The only change I can see from the first revision is that instead of `OS << getName()` it is now printing each character individually? This will be slower and more verbose than the original.

The `printName` function in `MCSectionELF.cpp` is iterating over individual characters in the name so that it can correctly quote them, e.g. if the name is `xx"yy` it will be printed as `"xx\"yy"`. Since this is a common operation (it's already copy-pasted verbatim to `MCSectionWasm.cpp`) it seems sensible to move it somewhere commonly accessible and call it from all three `MCSectionXXX.cpp`.

If you think doing that is outside the scope of this work then please restore the original version.


================
Comment at: llvm/lib/MC/MCContext.cpp:619-631
+  auto IterBool = GOFFUniquingMap.insert(
+      std::make_pair(GOFFSectionKey{Section.str()}, nullptr));
+  auto &Entry = *IterBool.first;
+  if (!IterBool.second)
+    return Entry.second;
+
+  // Otherwise, return a new section.
----------------
tmatheson wrote:
> Something like this should do the same, assuming the key is changed to `StringRef`
This comment still applies (even without getting rid of `GOFFSectionKey`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106380