[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: lib/Bitcode/Reader/BitcodeReader.cpp:2759
+        GET_OR_DISTINCT(DIChecksum,
+                        (Context, (DIChecksum::ChecksumType)Record[1],
+                         getMDString(Record[2]))),
----------------
aaboud wrote:
> 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.
I'm fine validating this in the IR itself, I'd be even fine leaving this field up to the backend which can only ignore and warn if it can't handle the type of the checksum (i.e. the IR verifier would accept null or any string for this field).

I raised it here because you put some effort into having a "robust" textual IR path while the bitcode was not well validated, which seems somehow backward.


https://reviews.llvm.org/D27642