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

[Amjad Aboud via Phabricator via llvm-commits]

aaboud added inline comments.


================
Comment at: docs/LangRef.rst:4018
+                     data: "000102030405060708090a0b0c0d0e0f")
+
 .. _DIBasicType:
----------------
mehdi_amini wrote:
> aaboud wrote:
> > mehdi_amini wrote:
> > > aaboud wrote:
> > > > aprantl wrote:
> > > > > aaboud wrote:
> > > > > > mehdi_amini wrote:
> > > > > > > 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.
> > > > > > > I rather have a much simpler implementation, the validation can be done in the verifier easily and it would handle similarly in-memory validation, bitcode and textual IR (the first two are the most important by the way, textual IR is not what we should optimize for).  
> > > > > > > 
> > > > > > > With the change that @rnk suggested that would make it quite a smaller patch I believe.
> > > > > > Fine with me.
> > > > > I'm in favor of rolling it into DIFile. As Paul noted, DWARF 5 also allows for an MD5 checksum on each file entry in the line table section so I'm expecting this to become the normal case eventually.
> > > > > 
> > > > > What's the problem with recognizing MD5 as a context-sensitive token in the parser?
> > > > It is not only MD5, right? should the LLexer recognize all possible types of checksum, i.e. MD5, SHA1, etc.?
> > > > Or like it is implemented in this patch, it only needs to recognize the checksum type prefix (whatever it will be).
> > > > 
> > > > So, how do you prefer to implement this?
> > > > 1. Prefix - (if yes, what should be the prefix?)
> > > > 2. Recognize all types
> > > > 3. Have a wild string
> > > Alternative representation:
> > > 
> > > `!DIFile(filename: ..., checksum: !"MD5:deadbeef")`
> > > 
> > > 
> > Once again, I do not mind how we implement it, I just need to hear an agreement from most of the participants in this review.
> > however, I think that with this approach it might complicate the code in CodeViewDebug, as it needs to parse the checksum string to get the type.
> > 
> > I would like to hear Reid opinion on this suggestion before I go and implement it.
> Is there already a patch up that shows the `CodeViewDebug`, so that I can see how it gets more complicated there?
> I'd expect it to be a `StringSwitch` on the prefix in the string instead of a switch on the enum value and that's it.
Sorry, I do not have this patch, and I am not planing to.
I hope that Reid can implement this in CodeViewDebug once I commit this patch.
This is why I would like to hear his agreement to your proposal before I go this direction.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:2759
+        GET_OR_DISTINCT(DIChecksum,
+                        (Context, (DIChecksum::ChecksumType)Record[1],
+                         getMDString(Record[2]))),
----------------
mehdi_amini wrote:
> aaboud wrote:
> > mehdi_amini wrote:
> > > What if the value in `Record[1]` is unexpected?
> > Do you suggest to do this instead?
> > 
> > ```
> > static_cast<DIChecksum::ChecksumType>(Record[1])
> > ```
> > 
> Not really, this is identical. What I mean is that the value in `Record[1]` can be totally invalid. 
> 
> You were concerned about the error checking in the LLParser while it is the least of my concern: the textual IR is more of a "debugging" representation while In Memory and Bitcode representation of the IR are both on the production path.
> 
1. I implemented the DIChecksumm::CheckSumType similar to DINode::DIFlags.
2. Do you think that if we use String in Record[1], we will be able to validate it here?
I think that it is fare enough to validate these values in the IR, after all LLexer and BitcodeReader are limited on how much they can validate.


https://reviews.llvm.org/D27642