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

[Anirudh Prasad via Phabricator via llvm-commits]

anirudhp marked an inline comment as done.
anirudhp added inline comments.


================
Comment at: llvm/include/llvm/MC/MCSectionGOFF.h:37
+                            const MCExpr *Subsection) const override {
+    OS << "\t.section\t\"" << getName() << "\"\n";
+  }
----------------
tmatheson wrote:
> 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.
Ahh I see what you mean. I think for now, I'd rather just keep it the way it was originally (i.e. not looping through it character by character). I would possibly keep the refactoring of the `printName` function so that it can be used by MCSection[ELF|GOFF|Wasm] as a separate patch if that's okay?


================
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:
> 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`)
Looking at it further. I followed your previous comment and removed the `GOFFSectionKey` struct and used as a standard `std::string` as the Key. We do have code that we plan to put up in the next series of patches, which would make a `StringRef` as the key not work, hence I opted for a standard `std::string`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106380