[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#619007, @mehdi_amini wrote:

> I think we try to make as much as possible metadata to be "distinct", CC Duncan to advise.


Not sure about that, so far we have only DICompileUnit and DISubprogram defined as distinct.
If I am not mistaken, distinct will prevent us from sharing Metadata, and for checksum there is no harm of sharing.



================
Comment at: docs/LangRef.rst:4018
+                     data: "000102030405060708090a0b0c0d0e0f")
+
 .. _DIBasicType:
----------------
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.







================
Comment at: include/llvm/IR/DebugInfoMetadata.h:430
   inline StringRef getDirectory() const;
+  inline DIChecksum *getChecksum() const;
 
----------------
mehdi_amini wrote:
> Why there? Where is this API used?
> 
We need to getChecksum from DICompileUnit, however, this information is stored in the DIScope.


================
Comment at: include/llvm/IR/DebugInfoMetadata.h:491
+                             bool ShouldCreate = true) {
+    // Do not create checksum with "None" checksum type.
+    if (Type == None)
----------------
mehdi_amini wrote:
> What is the reason to have the "None"?
We need to pass the checksum data and type to the DICompileUnit together with the filename and directory. However, not always we have a checksum data to send, and that is why we need an indicator that there is no checksum available.

If you try to create a checksum with None type Checksum::get() will return a nullptr.
This is the best way I could find to deal with the issue.
Please, let me know if you have better way to do that.


================
Comment at: include/llvm/IR/DebugInfoMetadata.h:1331
   StringRef getDirectory() const { return getScope()->getDirectory(); }
+  //DIChecksum *getChecksum() const { return getScope()->getChecksum(); }
 
----------------
mehdi_amini wrote:
> Intended?
I should have removed this line.
I am not sure why we have the getFilename() and getDirectory() for DILocation, thought that I might need the getChecksum for same reason, however, I guess it is not needed.

If for any reason we end up need this function we will add it then.
For now I will remove it.


https://reviews.llvm.org/D27642