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

[Eric Christopher via Phabricator via llvm-commits]

echristo added a comment.

In general, I think this patch needs a lot more description and possibly breaking up. For now a few questions:

a) nvptx doesn't allow a debug_str section?
b) relocations in nvptx should be against the section rather than the label? (To be fair, they can be this way on elf platforms as well which would be an improvement)
c) There are a bunch of directives for nvptx that you want to use for emission? 
d) It looks like you've got this turned off by default by commenting out everything? I'd rather you just conditionalize it and have it all work and be testable before turning on.

I think I'd probably start splitting up the NVPTX asm printer patch into separate and testable patches, perhaps by rewriting the existing line table handling and going from there?

I've also made some inline comments of things that were confusing or otherwise not sure about if you also want to tackle those.

Thanks!

-eric



================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:416
     DIE &Die, SmallVector<RangeSpan, 2> Ranges) {
-  if (Ranges.size() == 1) {
-    const auto &single = Ranges.front();
-    attachLowHighPC(Die, single.getStart(), single.getEnd());
+  if (Ranges.size() == 1 || DD->useSectionsAsReferences()) {
+    const auto &front = Ranges.front();
----------------
I don't believe that this is a necessary or sufficient check here. What's the point of it?


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:760
+    const TargetLoweringObjectFile &TLOF = Asm->getObjFileLowering();
+    if (DD->useSectionsAsReferences()) {
+      LabelBegin = TLOF.getDwarfInfoSection()->getBeginSymbol();
----------------
So there's a difference between a begin symbol for a section and the section itself. You're doing the former and not the latter it looks like? And also I think you're breaking the possibility of multiple cus in a single module.


================
Comment at: lib/Target/NVPTX/MCTargetDesc/NVPTXTargetStreamer.cpp:36
+                                        raw_ostream &OS) {
+  // FIXME: remove comment once debug info is properly supported.
+  assert(!SubSection && "SubSection is not null!");
----------------
This isn't really a condition? :)


================
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 needs more elaboration.


================
Comment at: lib/Target/NVPTX/MCTargetDesc/NVPTXTargetStreamer.cpp:43
+    OS << "//\t}\n";
+  if (Section != FI->getTextSection() && Section != FI->getDataSection() &&
+      Section != FI->getBSSSection()) {
----------------
I don't understand what's going on here?


================
Comment at: lib/Target/NVPTX/MCTargetDesc/NVPTXTargetStreamer.h:27
+
+  /// Record DWARF file directives for later output.
+  void emitDwarfFileDirective(StringRef Directive) override;
----------------
A description of how this works would be good.


================
Comment at: lib/Target/NVPTX/NVPTXAsmPrinter.cpp:875
 
-  if (MAI->doesSupportDebugInformation())
-    O << ", debug";
+  // FIXME: remove comment once debug info is properly supported.
+  if (MMI && MMI->hasDebugInfo())
----------------
I'm not sure what this fixme means?


Repository:
  rL LLVM

https://reviews.llvm.org/D41827