[PATCH] D27642: DebugInfo: Added support for Checksum debug info feature (LLVM IR part)

[Amjad Aboud via Phabricator via llvm-commits]

aaboud added a comment.

In https://reviews.llvm.org/D27642#619933, @rnk wrote:

> Any objections to folding this into DIFile? It would substantially reduce the size of this patch, IMO.


I have no objection going this way, if everybody agree on that I will implement the change and upload a new patch.



================
Comment at: docs/LangRef.rst:4018
+                     data: "000102030405060708090a0b0c0d0e0f")
+
 .. _DIBasicType:
----------------
rnk wrote:
> aaboud wrote:
> > mehdi_amini wrote:
> > > What are the valid list of "type"? What type is the "type" field? Why the prefix `ChecksumType_` and not just `MD5`?
> > > Why not a simpler approach where the type is an MDString? (That already how you parse it in DIChecksum::getChecksumType anyway)
> > Right now the valid list of types are:
> > ChecksumType_MD5
> > ChecksumType_SHA1
> > 
> > I will add this to the document.
> > 
> > Answering your other questions:
> > 1. The current solution does not handle the type as string, but as an enumeration with a known token (see LLexer.cpp)
> > 2. Omitting the prefix ChecksumType_ will not allow us to recognize the token.
> > 3. This is similar to what is done with DIFlag...
> > 4. Allowing the user to write the type as string "MD5" is easier to implement, however, checking invalid types will not be elegant as this solution.
> > 
> > Bottom line, I am ready to go with either solutions.
> > Let's hear what others think.
> > 
> > 
> > 
> > 
> > 
> I think we could fold DIChecksum into DIFile. It would look like:
>   !DIFile(filename: ..., checksumkind: ChecksumType_MD5, checksum: !"deadbeef")
> 
> This adds a dead integer to DIFile when checksums aren't being used, but it's probably more efficient and less work than having a separate node.
Fine with me.


================
Comment at: include/llvm/IR/DebugInfoMetadata.h:430
   inline StringRef getDirectory() const;
+  inline DIChecksum *getChecksum() const;
 
----------------
rnk wrote:
> aaboud wrote:
> > mehdi_amini wrote:
> > > Why there? Where is this API used?
> > > 
> > We need to getChecksum from DICompileUnit, however, this information is stored in the DIScope.
> I don't believe we really need it. We can probably do `CU->getFile()->getChecksum()`.
OK, can do.


================
Comment at: lib/IR/DIBuilder.cpp:127-128
+      VMContext, Lang,
+      DIFile::get(VMContext, Filename, Directory,
+                  DIChecksum::get(VMContext, ChecksumType, Checksum)),
+      Producer, isOptimized, Flags, RunTimeVer, SplitName, Kind, nullptr,
----------------
rnk wrote:
> Instead of having createCompileUnit take Filename, Directory, ChecksumType, and Checksum, have it take a DIFIle.
It would be much better solution for sure.
I will give it a try (in different patch), unless you want to do it yourself :) ?


https://reviews.llvm.org/D27642