[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:
----------------
aprantl wrote:
> aaboud 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.
> > 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


https://reviews.llvm.org/D27642