[PATCH] D42765: [DebugInfo] Support DWARFv5 source code embedding extension

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

Thank you both for the comments, I am switching dev machines so I will be a bit slow to implement the necessary changes.



================
Comment at: include/llvm/BinaryFormat/Dwarf.def:750
 HANDLE_DW_LNCT(0x05, MD5)
+HANDLE_DW_LNCT(0x2001, has_source)
+HANDLE_DW_LNCT(0x2002, source)
----------------
aprantl wrote:
> How did you come up with the number?
> Are you reasonably sure that no one else is using it?
> Perhaps add a prefix like LLVM_ or AMDGPU_ to the constants?
I am in the process of proposing this extension for inclusion in the next DWARF standard, but for now I will add a prefix.

I have not done much research on what values (if any) in the user-range are already in use, so I will try to find some references on that.


================
Comment at: include/llvm/IR/DIBuilder.h:151
+                       StringRef Checksum = StringRef(),
+                       Optional<StringRef> Source = None);
 
----------------
JDevlieghere wrote:
> I'm not a fan of these two things not being consistent. Either both should be options or both should be empty StringRefs? This seems to be recurring throughout this patch. I think `Optional` better conveys the idea, but would require more API changes. 
I agree, but would like feedback on the approach to fixing it.

The new code uses an `Optional` to explicitly support an empty source string, without it being assumed to simply be absent, so that `None != ""`. While it may not have many practical uses, it seems better to be explicit.

The old code is also explicit, but the paired `ChecksumKind` is what encodes whether a checksum is available. I do not want to duplicate this, as it leads to inconsistencies when `CSKind != CSK_None`, but `Checksum == None`, or vice-versa.

The best option I see is to wrap the pair in an optional (`Optional<pair<CSKind, Checksum>>`), remove the `CSK_None` variant, and maybe make the pair an explicit struct with members for the kind and checksum. Would this be an acceptable fix?


Repository:
  rL LLVM

https://reviews.llvm.org/D42765