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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
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.
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.


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




================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:2759
+        GET_OR_DISTINCT(DIChecksum,
+                        (Context, (DIChecksum::ChecksumType)Record[1],
+                         getMDString(Record[2]))),
----------------
What if the value in `Record[1]` is unexpected?


================
Comment at: lib/IR/Verifier.cpp:942
+  AssertDI(N.getTag() == dwarf::DW_TAG_null, "invalid tag", &N);
+  AssertDI(N.getType() != DIChecksum::None, "invalid checksum type", &N);
+}
----------------
The check could be more strict: what if the bitcode contained an invalid ID?


https://reviews.llvm.org/D27642