[PATCH] D41827: [DEBUG] Initial adaptation of NVPTX target for debug info emission.

[Eric Christopher via Phabricator via llvm-commits]

echristo added inline comments.


================
Comment at: lib/Target/NVPTX/MCTargetDesc/NVPTXMCAsmInfo.cpp:42
+  // FIXME: remove comment once debug info is properly supported.
+  Data8bitsDirective = "// .b8 ";
+  Data16bitsDirective = nullptr; // not supported
----------------
Do we still need these parts commented out?


================
Comment at: lib/Target/NVPTX/MCTargetDesc/NVPTXTargetStreamer.cpp:39
+  const MCObjectFileInfo *FI = getStreamer().getContext().getObjectFileInfo();
+  // Emit closing brace for DWARF sections only.
+  if (CurSection && CurSection != FI->getTextSection() &&
----------------
This should be commented as to "only dwarf sections need comments according to 'some reason'"


================
Comment at: lib/Target/NVPTX/MCTargetDesc/NVPTXTargetStreamer.cpp:39
+  const MCObjectFileInfo *FI = getStreamer().getContext().getObjectFileInfo();
+  // Emit closing brace for DWARF sections only.
+  if (CurSection && CurSection != FI->getTextSection() &&
----------------
echristo wrote:
> This needs more elaboration.
Similar to my comment below, I think that the "which section am I in" part needs some work. I definitely don't think it should be a "!=" comparison, and perhaps should be checking for some other metadata/feature.


================
Comment at: lib/Target/NVPTX/MCTargetDesc/NVPTXTargetStreamer.cpp:43
+    OS << "//\t}\n";
+  if (Section != FI->getTextSection() && Section != FI->getDataSection() &&
+      Section != FI->getBSSSection()) {
----------------
ABataev wrote:
> echristo wrote:
> > I don't understand what's going on here?
> The body of the DWARF sections in PTX file must be enclosed into braces, like this:
> ```
> .section .debug_info
> {
> ...
> }
> ```
> This code checks that current section is one of the DWARF sections and encloses the content of these sections into braces.
But why looking for particular sections this way? 


================
Comment at: lib/Target/NVPTX/MCTargetDesc/NVPTXTargetStreamer.h:33
+  /// same level as kernel and device function declarations.
+  /// LLVM emits .file directives immediately the location debug info is
+  /// emitted, i.e. they may be emitted inside functions. We gather all these
----------------
ABataev wrote:
> echristo wrote:
> > So, this functionality is used for changing files within the line table. I think what's actually going on here is that nvptx, for some reason, only supports a single file directive.
> > 
> > Basically you don't support the idea of:
> > 
> > int foo (void) {
> >   // some code
> >   #include "somefile.def"
> >   // some more code
> > }
> > 
> > having a file switch in the middle of a function. That's arguably a bug, but I think you don't want to collect them and emit them outside the functions because then it would be more actively wrong.
> No, it means that you may have the reference to the `somefile.def`, but the table of files should be emitted outside of the function, in the outer context
I'm not sure what you mean. Can you show some examples and elaborate? I.e. you may have a file table entry, but if you're not emitting them before the function you might have a bad time?

What do you do with this testcase?

echristo at athyra ~> cat foo.c
void foo() {
  #include "foo.def"
  return;
}
echristo at athyra ~> cat foo.def
#include <stdio.h>
printf("hello world\n");


================
Comment at: lib/Target/NVPTX/NVPTXAsmPrinter.h:350
+    bool Result = AsmPrinter::runOnMachineFunction(F);
+    OutStreamer->EmitRawText(StringRef("}\n"));
+    return Result;
----------------
ABataev wrote:
> tra wrote:
> > Do I understand it correctly that printout of the `}` was moved here out of `EmitFunctionBodyEnd()` so that we can emit additional debug labels after the last basic block?
> > 
> > It would be good to add few comments describing why we emit (or not) braces in particular places.
> Yes, correct.
> Ok, will add
Can you add a rationale here as to why we do it here rather than some other place? More detail in comment descriptions is good.


Repository:
  rL LLVM

https://reviews.llvm.org/D41827