Provide support for preserving assembly comments

[niravd]

Summary

Preserve assembly comments from input in output assembly and flags to
toggle property. This is on by default for inline assembly and off in
llvm-mc.

Parsed comments are emitted immediately before an EOL which generally
places them on the expected line.

[niravd]

Add llvm-mc test and deal with new LineComment structure

[niravd]

Transform preserved non-assembly comments to assembly-style comments

[majnemer]

What is the rationale behind adding this functionality to MC?

include/llvm/MC/MCParser/MCAsmLexer.h

27–85 ↗

(On Diff #59583)

Please do not reformat this enum in this differential.

[niravd]

In D20020#452526, @majnemer wrote:

What is the rationale behind adding this functionality to MC?

There are some projects which rely on emitted comments from inline assembly, e.g., Seabios. As a result when compiling with clang they turn off the integrated assembler.

[niravd]

Resolve comments. Change C comment handling to generate better multi-line comments.

[niravd]

Ping.

[rnk]

Hm, sorry, I never hit submit.

In D20020#452579, @niravd wrote:

In D20020#452526, @majnemer wrote:

What is the rationale behind adding this functionality to MC?

There are some projects which rely on emitted comments from inline assembly, e.g., Seabios. As a result when compiling with clang they turn off the integrated assembler.

I think we should support this functionality so that we can give a reasonable workaround to the Linux kernel people for this: https://llvm.org/bugs/show_bug.cgi?id=18891

include/llvm/MC/MCStreamer.h

281

This is only called from MCAsmStreamer. Should we sink it down there and devirtualize it, or do we think there will be clients other than EmitEOL?

include/llvm/Target/TargetOptions.h

191

This should really live in MCTargetOptions next to AsmVerbose, IMO.

lib/MC/MCAsmInfo.cpp

110

Why default to true?

[niravd]

Update in light of rnk's comments

[niravd]

include/llvm/MC/MCStreamer.h

281

I'm inclined to keep it here with the other outputting functions. That said, by that argument it probably should be renamed emitExplicitComments.

include/llvm/Target/TargetOptions.h

191

Make sense. Changed.

191

Agreed

lib/MC/MCAsmInfo.cpp

110

Since the integrated assembler goes from preprocessor assembly to preprocessed assembly with some nominal cleanup, it seems like if we're assuming comments may have semantic value the default should be preservation for people who haven't reasoned about it explicitly.

[rnk]

lgtm

lib/MC/MCAsmInfo.cpp

110

Also, I guess it's basically off by default if you emit an object file. OK.

include/llvm/MC/MCAsmInfo.h

protected:
 
   /// Should we use the integrated assembler?
   /// The integrated assembler should be enabled by default (by the
   /// constructors) when failing to parse a valid piece of assembly (inline
   /// or otherwise) is considered a bug. It may then be overridden after
   /// construction (see LLVMTargetMachine::initAsmInfo()).
   bool UseIntegratedAssembler;
 
+  /// Preserve Comments in assembly
+  bool PreserveAsmComments;
+
   /// Compress DWARF debug sections. Defaults to no compression.
   DebugCompressionType CompressDebugSections;
 
   /// True if the integrated assembler should interpret 'a >> b' constant
   /// expressions as logical rather than arithmetic.
   bool UseLogicalShr;
 
   // If true, emit GOTPCRELX/REX_GOTPCRELX instead of GOTPCREL, on
public:
   /// Return true if assembly (inline or otherwise) should be parsed.
   bool useIntegratedAssembler() const { return UseIntegratedAssembler; }
 
   /// Set whether assembly (inline or otherwise) should be parsed.
   virtual void setUseIntegratedAssembler(bool Value) {
     UseIntegratedAssembler = Value;
   }
 
+  /// Return true if assembly (inline or otherwise) should be parsed.
+  bool preserveAsmComments() const { return PreserveAsmComments; }
+
+  /// Set whether assembly (inline or otherwise) should be parsed.
+  virtual void setPreserveAsmComments(bool Value) {
+    PreserveAsmComments = Value;
+  }
+
   DebugCompressionType compressDebugSections() const {
     return CompressDebugSections;
   }
 
   void setCompressDebugSections(DebugCompressionType CompressDebugSections) {
     this->CompressDebugSections = CompressDebugSections;
   }
 
   bool shouldUseLogicalShr() const { return UseLogicalShr; }
 
   bool canRelaxRelocations() const { return RelaxELFRelocations; }
   void setRelaxELFRelocations(bool V) { RelaxELFRelocations = V; }
 };
 }
 
 #endif

include/llvm/MC/MCStreamer.h

public:
   /// \brief Return true if this asm streamer supports emitting unformatted text
   /// to the .s file with EmitRawText.
   virtual bool hasRawTextSupport() const { return false; }
 
   /// \brief Is the integrated assembler required for this streamer to function
   /// correctly?
   virtual bool isIntegratedAssemblerRequired() const { return false; }
 
-  /// \brief Add a textual command.
+  /// \brief Add a textual comment.
   ///
   /// Typically for comments that can be emitted to the generated .s
   /// file if applicable as a QoI issue to make the output of the compiler
   /// more readable.  This only affects the MCAsmStreamer, and only when
   /// verbose assembly output is enabled.
   ///
   /// If the comment includes embedded \n's, they will each get the comment
   /// prefix as appropriate.  The added comment should not end with a \n.
   virtual void AddComment(const Twine &T) {}
 
   /// \brief Return a raw_ostream that comments can be written to. Unlike
   /// AddComment, you are required to terminate comments with \n if you use this
   /// method.
   virtual raw_ostream &GetCommentOS();
 
   /// \brief Print T and prefix it with the comment string (normally #) and
   /// optionally a tab. This prints the comment immediately, not at the end of
   /// the current line. It is basically a safe version of EmitRawText: since it
   /// only prints comments, the object streamer ignores it instead of asserting.
   virtual void emitRawComment(const Twine &T, bool TabPrefix = true);
 
+  /// \brief Add explicit comment T. T is required to be a valid
+  /// comment in the output and does not need to be escaped.
+  virtual void addExplicitComment(const Twine &T);
+  /// \brief emit explicit comments
+  virtual void outputExplicitComments();
+
   /// AddBlankLine - Emit a blank line to a .s file to pretty it up.
   virtual void AddBlankLine() {}
 
   /// @}
 
   /// \name Symbol & Section Management
   /// @{
 

include/llvm/Target/TargetOptions.h

public:
 
     /// UseInitArray - Use .init_array instead of .ctors for static
     /// constructors.
     unsigned UseInitArray : 1;
 
     /// Disable the integrated assembler.
     unsigned DisableIntegratedAS : 1;
 
+    /// Disable the integrated assembler.
+    unsigned PreserveAsmComments : 1;
+
     /// Compress DWARF debug sections.
     unsigned CompressDebugSections : 1;
 
     unsigned RelaxELFRelocations : 1;
 
     /// Emit functions into separate sections.
     unsigned FunctionSections : 1;
 

lib/CodeGen/LLVMTargetMachine.cpp

void LLVMTargetMachine::initAsmInfo() {
   // Provide the user with a useful error message about what's wrong.
   assert(TmpAsmInfo && "MCAsmInfo not initialized. "
          "Make sure you include the correct TargetSelect.h"
          "and that InitializeAllTargetMCs() is being invoked!");
 
   if (Options.DisableIntegratedAS)
     TmpAsmInfo->setUseIntegratedAssembler(false);
 
+  TmpAsmInfo->setPreserveAsmComments(Options.PreserveAsmComments);
+
   if (Options.CompressDebugSections)
     TmpAsmInfo->setCompressDebugSections(DebugCompressionType::DCT_ZlibGnu);
 
   TmpAsmInfo->setRelaxELFRelocations(Options.RelaxELFRelocations);
 
   if (Options.ExceptionModel != ExceptionHandling::None)
     TmpAsmInfo->setExceptionsType(Options.ExceptionModel);
 

lib/MC/MCAsmInfo.cpp

MCAsmInfo::MCAsmInfo() {
   // - Windows always enables the integrated assembler by default
   //   - MCAsmInfoCOFF is handling this case, should it be MCAsmInfoMicrosoft?
   // - MachO targets always enables the integrated assembler by default
   //   - MCAsmInfoDarwin is handling this case
   // - Generic_GCC toolchains enable the integrated assembler on a per
   //   architecture basis.
   //   - The target subclasses for AArch64, ARM, and X86 handle these cases
   UseIntegratedAssembler = false;
+  PreserveAsmComments = true;
 
   CompressDebugSections = DebugCompressionType::DCT_None;
 }
 
 MCAsmInfo::~MCAsmInfo() {
 }
 
 bool MCAsmInfo::isSectionAtomizableBySymbols(const MCSection &Section) const {

lib/MC/MCAsmStreamer.cpp

 class MCAsmStreamer final : public MCStreamer {
   std::unique_ptr<formatted_raw_ostream> OSOwner;
   formatted_raw_ostream &OS;
   const MCAsmInfo *MAI;
   std::unique_ptr<MCInstPrinter> InstPrinter;
   std::unique_ptr<MCCodeEmitter> Emitter;
   std::unique_ptr<MCAsmBackend> AsmBackend;
 
+  SmallString<128> ExplicitCommentToEmit;
   SmallString<128> CommentToEmit;
   raw_svector_ostream CommentStream;
 
   unsigned IsVerboseAsm : 1;
   unsigned ShowInst : 1;
   unsigned UseDwarfDirectory : 1;
 
   void EmitRegisterName(int64_t Register);
MCAsmStreamer(MCContext &Context, std::unique_ptr<formatted_raw_ostream> os,
         IsVerboseAsm(isVerboseAsm), ShowInst(showInst),
         UseDwarfDirectory(useDwarfDirectory) {
     assert(InstPrinter);
     if (IsVerboseAsm)
         InstPrinter->setCommentStream(CommentStream);
   }
 
   inline void EmitEOL() {
+    // Dump Explicit Comments here.
+    outputExplicitComments();
     // If we don't have any comments, just emit a \n.
     if (!IsVerboseAsm) {
       OS << '\n';
       return;
     }
     EmitCommentsAndEOL();
   }
 
public:
   raw_ostream &GetCommentOS() override {
     if (!IsVerboseAsm)
       return nulls();  // Discard comments unless in verbose asm mode.
     return CommentStream;
   }
 
   void emitRawComment(const Twine &T, bool TabPrefix = true) override;
 
+  void addExplicitComment(const Twine &T) override;
+  void outputExplicitComments() override;
+
   /// AddBlankLine - Emit a blank line to a .s file to pretty it up.
   void AddBlankLine() override {
     EmitEOL();
   }
 
   /// @name MCStreamer Interface
   /// @{
 

 
 void MCAsmStreamer::emitRawComment(const Twine &T, bool TabPrefix) {
   if (TabPrefix)
     OS << '\t';
   OS << MAI->getCommentString() << T;
   EmitEOL();
 }
 
+void MCAsmStreamer::addExplicitComment(const Twine &T) {
+  StringRef c = T.getSingleStringRef();
+  if (c.equals(StringRef(MAI->getSeparatorString())))
+    return;
+  if (c.startswith(StringRef("//"))) {
+    ExplicitCommentToEmit.append("\t");
+    ExplicitCommentToEmit.append(MAI->getCommentString());
+    // drop //
+    ExplicitCommentToEmit.append(c.slice(2, c.size()).str());
+  } else if (c.startswith(StringRef("/*"))) {
+    size_t p = 2, len = c.size() - 2;
+    // emit each line in comment as separate newline.
+    do {
+      size_t newp = std::min(len, c.find_first_of("\r\n", p));
+      ExplicitCommentToEmit.append("\t");
+      ExplicitCommentToEmit.append(MAI->getCommentString());
+      ExplicitCommentToEmit.append(c.slice(p, newp).str());
+      // If we have another line in this comment add line
+      if (newp < len)
+        ExplicitCommentToEmit.append("\n");
+      p = newp + 1;
+    } while (p < len);
+  } else if (c.startswith(StringRef(MAI->getCommentString()))) {
+    ExplicitCommentToEmit.append("\t");
+    ExplicitCommentToEmit.append(c.str());
+  } else if (c.front() == '#') {
+    // # are comments for ## commentString. Output extra #.
+    ExplicitCommentToEmit.append("\t#");
+    ExplicitCommentToEmit.append(c.str());
+  } else
+    assert(false && "Unexpected Assembly Comment");
+  // full line comments immediately output
+  if (c.back() == '\n')
+    outputExplicitComments();
+}
+
+void MCAsmStreamer::outputExplicitComments() {
+  StringRef Comments = ExplicitCommentToEmit;
+  if (!Comments.empty())
+    OS << Comments;
+  ExplicitCommentToEmit.clear();
+}
+
 void MCAsmStreamer::ChangeSection(MCSection *Section,
                                   const MCExpr *Subsection) {
   assert(Section && "Cannot switch to a null section!");
   Section->PrintSwitchToSection(*MAI, OS, Subsection);
 }
 
 void MCAsmStreamer::EmitLabel(MCSymbol *Symbol) {
   assert(Symbol->isUndefined() && "Cannot define a symbol twice!");

 void MCAsmStreamer::EmitSymbolDesc(MCSymbol *Symbol, unsigned DescValue) {
   OS << ".desc" << ' ';
   Symbol->print(OS, MAI);
   OS << ',' << DescValue;
   EmitEOL();
 }
 
 void MCAsmStreamer::EmitSyntaxDirective() {
-  if (MAI->getAssemblerDialect() == 1)
-    OS << "\t.intel_syntax noprefix\n";
+  if (MAI->getAssemblerDialect() == 1) {
+    OS << "\t.intel_syntax noprefix";
+    EmitEOL();
+  }
   // FIXME: Currently emit unprefix'ed registers.
   // The intel_syntax directive has one optional argument 
   // with may have a value of prefix or noprefix.
 }
 
 void MCAsmStreamer::BeginCOFFSymbolDef(const MCSymbol *Symbol) {
   OS << "\t.def\t ";
   Symbol->print(OS, MAI);

lib/MC/MCParser/AsmParser.cpp

void AsmParser::jumpToLoc(SMLoc Loc, unsigned InBuffer) {
   Lexer.setBuffer(SrcMgr.getMemoryBuffer(CurBuffer)->getBuffer(),
                   Loc.getPointer());
 }
 
 const AsmToken &AsmParser::Lex() {
   if (Lexer.getTok().is(AsmToken::Error))
     Error(Lexer.getErrLoc(), Lexer.getErr());
 
+  // if it's a end of statement with a comment in it
+  if (getTok().is(AsmToken::EndOfStatement)) {
+    // if this is a line comment output it.
+    if (getTok().getString().front() != '\n' &&
+        getTok().getString().front() != '\r' && MAI.preserveAsmComments())
+      Out.addExplicitComment(Twine(getTok().getString()));
+  }
+
   const AsmToken *tok = &Lexer.Lex();
-  // Drop comments here.
+
+  // Parse comments here to be deferred until end of next statement.
   while (tok->is(AsmToken::Comment)) {
+    if (MAI.preserveAsmComments())
+      Out.addExplicitComment(Twine(tok->getString()));
     tok = &Lexer.Lex();
   }
 
   if (tok->is(AsmToken::Eof)) {
     // If this is the end of an included file, pop the parent file off the
     // include stack.
     SMLoc ParentIncludeLoc = SrcMgr.getParentIncludeLoc(CurBuffer);
     if (ParentIncludeLoc != SMLoc()) {

lib/MC/MCParser/ELFAsmParser.cpp

 
 bool ELFAsmParser::ParseSectionSwitch(StringRef Section, unsigned Type,
                                       unsigned Flags, SectionKind Kind) {
   const MCExpr *Subsection = nullptr;
   if (getLexer().isNot(AsmToken::EndOfStatement)) {
     if (getParser().parseExpression(Subsection))
       return true;
   }
+  Lex();
 
   getStreamer().SwitchSection(getContext().getELFSection(Section, Type, Flags),
                               Subsection);
 
   return false;
 }
 
 bool ELFAsmParser::ParseDirectiveSize(StringRef, SMLoc) {

lib/MC/MCStreamer.cpp

 
 raw_ostream &MCStreamer::GetCommentOS() {
   // By default, discard comments.
   return nulls();
 }
 
 void MCStreamer::emitRawComment(const Twine &T, bool TabPrefix) {}
 
+void MCStreamer::addExplicitComment(const Twine &T) {}
+void MCStreamer::outputExplicitComments() {}
+
 void MCStreamer::generateCompactUnwindEncodings(MCAsmBackend *MAB) {
   for (auto &FI : DwarfFrameInfos)
     FI.CompactUnwindEncoding =
         (MAB ? MAB->generateCompactUnwindEncoding(FI.Instructions) : 0);
 }
 
 /// EmitIntValue - Special case of EmitValue that avoids the client having to
 /// pass in a MCExpr for constant integers.

test/MC/AsmParser/inline-comments.ll

+; RUN: llc %s -o - | sed -n -e '/#APP/,/#NO_APP/p' > %t
+; RUN: sed -n -e 's/^;CHECK://p' %s > %t2
+; RUN: diff %t %t2
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: nounwind uwtable
+define void @foo() #0 {
+entry:
+  call void asm sideeffect "", "~{dirflag},~{fpsr},~{flags}"() #0
+;CHECK: #APP
+;CHECK: #NO_APP
+  call void asm sideeffect " ", "~{dirflag},~{fpsr},~{flags}"() #0
+;CHECK: #APP
+;CHECK:
+;CHECK: #NO_APP
+  call void asm sideeffect "\0A", "~{dirflag},~{fpsr},~{flags}"() #0
+;CHECK: #APP
+;CHECK:
+;CHECK:
+;CHECK: #NO_APP
+  call void asm sideeffect "/*isolated c comment*/", "~{dirflag},~{fpsr},~{flags}"() #0
+;CHECK: #APP
+;CHECK: #isolated c comment
+;CHECK: #NO_APP
+  call void asm sideeffect "/**/", "~{dirflag},~{fpsr},~{flags}"() #0
+;CHECK: #APP
+;CHECK: #
+;CHECK: #NO_APP
+  call void asm sideeffect "/*comment with\0Anewline*/", "~{dirflag},~{fpsr},~{flags}"() #0
+;CHECK: #APP
+;CHECK: #comment with
+;CHECK: #newline
+;CHECK: #NO_APP
+  call void asm sideeffect "//isolated line comment", "~{dirflag},~{fpsr},~{flags}"() #0
+;CHECK: #APP
+;CHECK: #isolated line comment
+;CHECK: #NO_APP
+  call void asm sideeffect "#isolated line comment", "~{dirflag},~{fpsr},~{flags}"() #0
+;CHECK: #APP
+;CHECK: #isolated line comment
+;CHECK: #NO_APP
+   call void asm sideeffect "nop /* after nop */", "~{dirflag},~{fpsr},~{flags}"() #0
+;CHECK: #APP
+;CHECK: nop # after nop 
+;CHECK: #NO_APP
+  call void asm sideeffect "nop // after nop", "~{dirflag},~{fpsr},~{flags}"() #0
+;CHECK: #APP
+;CHECK: nop # after nop
+;CHECK: #NO_APP
+  call void asm sideeffect "nop # after nop", "~{dirflag},~{fpsr},~{flags}"() #0
+;CHECK: #APP
+;CHECK: nop # after nop
+;CHECK: #NO_APP
+  call void asm sideeffect "nop /* after explicit ended nop */", "~{dirflag},~{fpsr},~{flags}"() #0
+;CHECK: #APP
+;CHECK: nop # after explicit ended nop 
+;CHECK: #NO_APP
+  call void asm sideeffect "nop # after explicit ended nop", "~{dirflag},~{fpsr},~{flags}"() #0
+;CHECK: #APP
+;CHECK: nop # after explicit ended nop
+;CHECK: #NO_APP
+  call void asm sideeffect "nop # after explicit end nop", "~{dirflag},~{fpsr},~{flags}"() #0
+;CHECK: #APP
+;CHECK: nop # after explicit end nop
+;CHECK: #NO_APP
+  call void asm sideeffect "/* before nop */ nop", "~{dirflag},~{fpsr},~{flags}"() #0
+;CHECK: #APP
+;CHECK: nop # before nop 
+;CHECK: #NO_APP
+  call void asm sideeffect "//comment with escaped newline\0A", "~{dirflag},~{fpsr},~{flags}"() #0
+;CHECK: #APP
+;CHECK: #comment with escaped newline
+;CHECK:
+;CHECK: #NO_APP
+  call void asm sideeffect "/*0*/xor/*1*/%eax,/*2*/%ecx/*3*///eol", "~{dirflag},~{fpsr},~{flags}"() #0
+;CHECK: #APP
+;CHECK: xorl  %eax, %ecx  #0  #1  #2  #3  #eol
+;CHECK: #NO_APP
+  ret void
+}
+
+attributes #0 = { nounwind }
+
+!llvm.ident = !{!0}
+
+!0 = !{!"clang version 3.9.0 (trunk 268625) (llvm/trunk 268631)"}

test/MC/AsmParser/preserve-comments.s

+  #RUN: llvm-mc -preserve-comments -n -triple i386-linux-gnu < %s > %t
+  #RUN: diff %s %t
+  .text
+
+  nop
+  #if DIRECTIVE COMMENT
+  ## WHOLE LINE COMMENT
+  cmpl  $196, %eax  ## EOL COMMENT
+  #endif

tools/llc/llc.cpp

 TimeCompilations("time-compilations", cl::Hidden, cl::init(1u),
                  cl::value_desc("N"),
                  cl::desc("Repeat compilation N times for timing"));
 
 static cl::opt<bool>
 NoIntegratedAssembler("no-integrated-as", cl::Hidden,
                       cl::desc("Disable integrated assembler"));
 
+static cl::opt<bool>
+    NoPreserveComments("fno-preserve-as-comments", cl::Hidden,
+                       cl::desc("Preserve Comments in outputted assembly"));
+
 // Determine optimization level.
 static cl::opt<char>
 OptLevel("O",
          cl::desc("Optimization level. [-O0, -O1, -O2, or -O3] "
                   "(default = '-O2')"),
          cl::Prefix,
          cl::ZeroOrMore,
          cl::init(' '));
static int compileModule(char **argv, LLVMContext &Context) {
   case '0': OLvl = CodeGenOpt::None; break;
   case '1': OLvl = CodeGenOpt::Less; break;
   case '2': OLvl = CodeGenOpt::Default; break;
   case '3': OLvl = CodeGenOpt::Aggressive; break;
   }
 
   TargetOptions Options = InitTargetOptionsFromCodeGenFlags();
   Options.DisableIntegratedAS = NoIntegratedAssembler;
+  Options.PreserveAsmComments = !NoPreserveComments;
   Options.MCOptions.ShowMCEncoding = ShowMCEncoding;
   Options.MCOptions.MCUseDwarfDirectory = EnableDwarfDirectory;
   Options.MCOptions.AsmVerbose = AsmVerbose;
 
   std::unique_ptr<TargetMachine> Target(
       TheTarget->createTargetMachine(TheTriple.getTriple(), CPUStr, FeaturesStr,
                                      Options, getRelocModel(), CMModel, OLvl));
 

tools/llvm-mc/llvm-mc.cpp

 
 static cl::opt<bool>
 PrintImmHex("print-imm-hex", cl::init(false),
             cl::desc("Prefer hex format for immediate values"));
 
 static cl::list<std::string>
 DefineSymbol("defsym", cl::desc("Defines a symbol to be an integer constant"));
 
+static cl::opt<bool>
+    PreserveComments("preserve-comments",
+                     cl::desc("Preserve Comments in outputted assembly"));
+
 enum OutputFileType {
   OFT_Null,
   OFT_AssemblyFile,
   OFT_ObjectFile
 };
 static cl::opt<OutputFileType>
 FileType("filetype", cl::init(OFT_AssemblyFile),
   cl::desc("Choose an output file type:"),
int main(int argc, char **argv) {
   if (CompressDebugSections != DebugCompressionType::DCT_None) {
     if (!zlib::isAvailable()) {
       errs() << ProgName
              << ": build tools with zlib to enable -compress-debug-sections";
       return 1;
     }
     MAI->setCompressDebugSections(CompressDebugSections);
   }
+  MAI->setPreserveAsmComments(PreserveComments);
 
   // FIXME: This is not pretty. MCContext has a ptr to MCObjectFileInfo and
   // MCObjectFileInfo needs a MCContext reference in order to initialize itself.
   MCObjectFileInfo MOFI;
   MCContext Ctx(MAI.get(), MRI.get(), &MOFI, &SrcMgr);
   MOFI.InitMCObjectFileInfo(TheTriple, PIC, CMModel, Ctx);
 
   if (SaveTempLabels)