[NVPTX] Initial adaptation of MCAsmStreamer/MCTargetStreamer for debug info in Cuda.

[ABataev]

Summary

Initial changes in interfaces of MCAsmStreamer/MCTargetStreamer for
correct debug info emission for Cuda.

1. PTX foramt does not support .ascii directives. Added the ability to

nullify it.

2. DWARF .file directives must be emitted after .text section and

cannot be inlined in function bodies.

3. The initial function label must follow the first debug .loc

directive, not be followed by.

4. DWARF sections must be enclosed in braces.

[ABataev]

Ping

[ABataev]

Ping

[ABataev]

Ping

[echristo]

This needs careful review. I'll take a look as soon as I can.

[gchakrabarti]

What do you mean by this:

"DWARF .loc directives must be emitted after .text section and
cannot be inlined in function bodies."

[ABataev]

In D40033#945457, @gchakrabarti wrote:

What do you mean by this:

"DWARF .loc directives must be emitted after .text section and
cannot be inlined in function bodies."

Sorry, my mistake. I mean .file directive.

[tra]

LGTM in general. Can you think of a way to verify new functionality? It looks like we may have to wait until some target starts using it.

lib/MC/MCAsmStreamer.cpp

1101 ↗

(On Diff #122857)

We do not print EOL after the file name any more. Is that intentional?

lib/MC/MCStreamer.cpp

62–64 ↗

(On Diff #122857)

Do you have further plans to do something more elaborate here? Otherwise I'd remove it and just use EmitRawText.

[tra]

I'll defer to @echristo for the final approval.

[ABataev]

lib/MC/MCAsmStreamer.cpp

1101 ↗

(On Diff #122857)

EmitRawText emits EOL automatically.

lib/MC/MCStreamer.cpp

62–64 ↗

(On Diff #122857)

Sure, I have plans. As soon as this patch is accepted, I'm going to publish next patches with debug info support for NVPTX. This is just the first part.

[ABataev]

In D40033#945474, @tra wrote:

I'll defer to @echristo for the final approval.

Sure, that's why added Eric as a reviewer. :) Some changes may brake codegen for non-NVPTX targets and we have to be sure that everything works correctly everywhere.

[gchakrabarti]

Note that PTX allows .file even before .text. The spec is accurate here:

"The .file directive is allowed only in the outermost scope, i.e., at the same level as kernel and device function declarations."

The reason .file may be needed after .text is that if while emitting the .text, you do not know the file directives yet.

[ABataev]

In D40033#945486, @gchakrabarti wrote:

Note that PTX allows .file even before .text. The spec is accurate here:

"The .file directive is allowed only in the outermost scope, i.e., at the same level as kernel and device function declarations."

The reason .file may be needed after .text is that if while emitting the .text, you do not know the file directives yet.

Yes, you're right. That's the idea.

[echristo]

test/CodeGen/Mips/mips16ex.ll

6 ↗

(On Diff #122857)

For the record this change is the one I'm concerned about. I don't think it's desired to put the cfi directives before the symbol.

That said, I'm going to have to take a closer look.

[ABataev]

test/CodeGen/Mips/mips16ex.ll

6 ↗

(On Diff #122857)

Yes, I understand this. It is required in PTX to have .loc before $func_begin symbol, otherwise it emits incorrect debug info. At least, we can make this change only for Cuda.

[ABataev]

test/CodeGen/Mips/mips16ex.ll

6 ↗

(On Diff #122857)

Eric, any news?

[jlebar]

test/CodeGen/Mips/mips16ex.ll

6 ↗

(On Diff #122857)

Proxying ecirhsto for a moment, since he's currently slammed with something else: Your suggestion of doing this just for CUDA would resolve a lot of the fears here.

[ABataev]

Removed emission of the function start label after first debug .loc directive. Will add it later with the test.

[jlebar]

(Is this ready for someone to have another look?)

[jlebar]

(Is this ready for someone to have another look?)

(I'm pretty motivated to help you get the reviews you need on this one and the other NVPTX debuginfo patches, so don't be shy to ping me here or directly over email if you're not getting movement from someone at Google. But FYI I'll be gone on vacation from Dec 24 to Jan 7.)

[ABataev]

In D40033#958932, @jlebar wrote:

(Is this ready for someone to have another look?)

(I'm pretty motivated to help you get the reviews you need on this one and the other NVPTX debuginfo patches, so don't be shy to ping me here or directly over email if you're not getting movement from someone at Google. But FYI I'll be gone on vacation from Dec 24 to Jan 7.)

Are you ok with this patch? If so, I will commit it and start to prepare the next patches

[jlebar]

Are you ok with this patch? If so, I will commit it and start to prepare the next patches

Judging by the fact that you don't have to change any tests here, it looks like this does not change debuginfo for any platforms. My understanding is this was echristo's main concern. LGTM.

llvm/trunk/include/llvm/MC/MCAsmInfo.h

protected:
 
   /// This should be set to the directive used to get some number of zero bytes
   /// emitted to the current section.  Common cases are "\t.zero\t" and
   /// "\t.space\t".  If this is set to null, the Data*bitsDirective's will be
   /// used to emit zero bytes.  Defaults to "\t.zero\t"
   const char *ZeroDirective;
 
   /// This directive allows emission of an ascii string with the standard C
-  /// escape characters embedded into it.  Defaults to "\t.ascii\t"
+  /// escape characters embedded into it.  If a target doesn't support this, it
+  /// can be set to null. Defaults to "\t.ascii\t"
   const char *AsciiDirective;
 
   /// If not null, this allows for special handling of zero terminated strings
   /// on this target.  This is commonly supported as ".asciz".  If a target
   /// doesn't support this, it can be set to null.  Defaults to "\t.asciz\t"
   const char *AscizDirective;
 
   /// These directives are used to output some unit of integer data to the

llvm/trunk/include/llvm/MC/MCStreamer.h

public:
   // Allow a target to add behavior to the EmitLabel of MCStreamer.
   virtual void emitLabel(MCSymbol *Symbol);
   // Allow a target to add behavior to the emitAssignment of MCStreamer.
   virtual void emitAssignment(MCSymbol *Symbol, const MCExpr *Value);
 
   virtual void prettyPrintAsm(MCInstPrinter &InstPrinter, raw_ostream &OS,
                               const MCInst &Inst, const MCSubtargetInfo &STI);
 
+  virtual void emitDwarfFileDirective(StringRef Directive);
+
+  /// Update streamer for a new active section.
+  ///
+  /// This is called by PopSection and SwitchSection, if the current
+  /// section changes.
+  virtual void changeSection(const MCSection *CurSection, MCSection *Section,
+                             const MCExpr *SubSection, raw_ostream &OS);
+
+  virtual void emitValue(const MCExpr *Value);
+
   virtual void finish();
 };
 
 // FIXME: declared here because it is used from
 // lib/CodeGen/AsmPrinter/ARMException.cpp.
 class ARMTargetStreamer : public MCTargetStreamer {
 public:
   ARMTargetStreamer(MCStreamer &S);

llvm/trunk/lib/MC/MCAsmStreamer.cpp

void MCAsmStreamer::emitExplicitComments() {
   if (!Comments.empty())
     OS << Comments;
   ExplicitCommentToEmit.clear();
 }
 
 void MCAsmStreamer::ChangeSection(MCSection *Section,
                                   const MCExpr *Subsection) {
   assert(Section && "Cannot switch to a null section!");
+  if (MCTargetStreamer *TS = getTargetStreamer()) {
+    TS->changeSection(getCurrentSectionOnly(), Section, Subsection, OS);
+  } else {
     Section->PrintSwitchToSection(
         *MAI, getContext().getObjectFileInfo()->getTargetTriple(), OS,
         Subsection);
   }
+}
 
 void MCAsmStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc) {
   MCStreamer::EmitLabel(Symbol, Loc);
 
   Symbol->print(OS, MAI);
   OS << MAI->getLabelSuffix();
 
   EmitEOL();
static void PrintQuotedString(StringRef Data, raw_ostream &OS) {
   OS << '"';
 }
 
 void MCAsmStreamer::EmitBytes(StringRef Data) {
   assert(getCurrentSectionOnly() &&
          "Cannot emit contents before setting section!");
   if (Data.empty()) return;
 
-  if (Data.size() == 1) {
-    OS << MAI->getData8bitsDirective();
-    OS << (unsigned)(unsigned char)Data[0];
+  // If only single byte is provided or no ascii or asciz directives is
+  // supported, emit as vector of 8bits data.
+  if (Data.size() == 1 ||
+      !(MAI->getAscizDirective() || MAI->getAsciiDirective())) {
+    const char *Directive = MAI->getData8bitsDirective();
+    for (const unsigned char C : Data.bytes()) {
+      OS << Directive << (unsigned)C;
       EmitEOL();
+    }
     return;
   }
 
   // If the data ends with 0 and the target supports .asciz, use it, otherwise
   // use .ascii
   if (MAI->getAscizDirective() && Data.back() == 0) {
     OS << MAI->getAscizDirective();
     Data = Data.substr(0, Data.size()-1);
for (unsigned Emitted = 0; Emitted != Size;) {
       EmitIntValue(ValueToEmit, EmissionSize);
       Emitted += EmissionSize;
     }
     return;
   }
 
   assert(Directive && "Invalid size for machine code value!");
   OS << Directive;
+  if (MCTargetStreamer *TS = getTargetStreamer()) {
+    TS->emitValue(Value);
+  } else {
     Value->print(OS, MAI);
     EmitEOL();
   }
+}
 
 void MCAsmStreamer::EmitULEB128Value(const MCExpr *Value) {
   int64_t IntValue;
   if (Value->evaluateAsAbsolute(IntValue)) {
     EmitULEB128IntValue(IntValue);
     return;
   }
   OS << ".uleb128 ";
if (!UseDwarfDirectory && !Directory.empty()) {
     else {
       FullPathName = Directory;
       sys::path::append(FullPathName, Filename);
       Directory = "";
       Filename = FullPathName;
     }
   }
 
-  OS << "\t.file\t" << FileNo << ' ';
+  SmallString<128> Str;
+  raw_svector_ostream OS1(Str);
+  OS1 << "\t.file\t" << FileNo << ' ';
   if (!Directory.empty()) {
-    PrintQuotedString(Directory, OS);
-    OS << ' ';
+    PrintQuotedString(Directory, OS1);
+    OS1 << ' ';
+  }
+  PrintQuotedString(Filename, OS1);
+  if (MCTargetStreamer *TS = getTargetStreamer()) {
+    TS->emitDwarfFileDirective(OS1.str());
+  } else {
+    EmitRawText(OS1.str());
   }
-  PrintQuotedString(Filename, OS);
-  EmitEOL();
 
   return FileNo;
 }
 
 void MCAsmStreamer::EmitDwarfLocDirective(unsigned FileNo, unsigned Line,
                                           unsigned Column, unsigned Flags,
                                           unsigned Isa,
                                           unsigned Discriminator,

llvm/trunk/lib/MC/MCStreamer.cpp

 
 // Pin the vtables to this file.
 MCTargetStreamer::~MCTargetStreamer() = default;
 
 void MCTargetStreamer::emitLabel(MCSymbol *Symbol) {}
 
 void MCTargetStreamer::finish() {}
 
+void MCTargetStreamer::changeSection(const MCSection *CurSection,
+                                     MCSection *Section,
+                                     const MCExpr *Subsection,
+                                     raw_ostream &OS) {
+  Section->PrintSwitchToSection(
+      *Streamer.getContext().getAsmInfo(),
+      Streamer.getContext().getObjectFileInfo()->getTargetTriple(), OS,
+      Subsection);
+}
+
+void MCTargetStreamer::emitDwarfFileDirective(StringRef Directive) {
+  Streamer.EmitRawText(Directive);
+}
+
+void MCTargetStreamer::emitValue(const MCExpr *Value) {
+  SmallString<128> Str;
+  raw_svector_ostream OS(Str);
+
+  Value->print(OS, Streamer.getContext().getAsmInfo());
+  Streamer.EmitRawText(OS.str());
+}
+
 void MCTargetStreamer::emitAssignment(MCSymbol *Symbol, const MCExpr *Value) {}
 
 MCStreamer::MCStreamer(MCContext &Ctx)
     : Context(Ctx), CurrentWinFrameInfo(nullptr) {
   SectionStack.push_back(std::pair<MCSectionSubPair, MCSectionSubPair>());
 }
 
 MCStreamer::~MCStreamer() {}