[PATCH] D45024: [DebugInfo] Add DILabel metadata and intrinsic llvm.dbg.label.

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
Comment at: lib/IR/AsmWriter.cpp:1978
+  Printer.printString("name", N->getName());
+  Printer.printMetadata("file", N->getRawFile());
+  Printer.printInt("line", N->getLine());
----------------
aprantl wrote:
> chenwj wrote:
> > HsiangKai wrote:
> > > aprantl wrote:
> > > > I think file and line should just come from the DILocation. Is there a situation where the File in the DILabel would not match the one in the DILocation?
> > > The line number in DILocation is used for Instruction’s location. The line number is DILabel is used for Label’s location. Their meanings are different. They are just happened to be the same.
> > > 
> > > I reviewed llvm.dbg.declare implementation. It seems that the line number in DILocalVariable is also redundant. I am not very clear why there existed redundant line information in DILocalVariable. The only reason I guess is they have different meanings in logic and the implementation is easier if metadata has line number in itself. Actually, the line number of variable is got from variable’s metadata instead of derived from its intrinsic’s DILocation.
> > > 
> > > There may be other reasons I did not aware. However, if it is really redundant. I think there should be another patch to review the design in all metadata instead of in this patch. I prefer to be consistent with other metadata design concept.
> > I agree with your last point. We can have another patch doing the cleanup, that makes things clear.
> > I reviewed llvm.dbg.declare implementation. It seems that the line number in DILocalVariable is also redundant. I am not very clear why there existed redundant line information in DILocalVariable. The only reason I guess is they have different meanings in logic and the implementation is easier if metadata has line number in itself. Actually, the line number of variable is got from variable’s metadata instead of derived from its intrinsic’s DILocation.
> 
> For llvm.dbg.value, the line number stored in DILocalVariable is used to set the DW_AT_decl_line of the variable information in the .debug_info section. The DILocation of the various intrinsic calls that describe locations for this variable is currently unused except for the `inliendAt:` part. The line number in the DILocalVariable is needed for variables that are entirely optimized away: in this case no debug info intrinsic is left and the DILocationVariable is only reachable via the DISubprogram's list of local variables.
> 
> In the case of DILabel, I don't think there is any value in keeping around labels that were entirely optimized away, so there is no harm in depending on the information stored in the DILocation attachment of the intrinsic. DO you think this is reasonable?
> 
> > There may be other reasons I did not aware. However, if it is really redundant. I think there should be another patch to review the design in all metadata instead of in this patch. I prefer to be consistent with other metadata design concept.
> 
> I disagree with this approach, both for the reasons outlined above, and because for every metadata change we also need to implement a bitcode upgrade in metadataloader which we need to carry around for an indefinite time.
If a label is optimized away, do we still emit the DW_TAG_label?  If so, then the source location should be kept in the DILabel and not rely on the intrinsic.  Otherwise the DWARF will have a DW_TAG_label with a name and no other information at all.


Repository:
  rL LLVM

https://reviews.llvm.org/D45024