r220493 - Add a "signature" to AST files to verify that they haven't changed

[Ben Langmuir]

> On Mar 23, 2015, at 5:38 PM, Chandler Carruth <chandlerc at google.com> wrote:
> 
> Digging this thread back up, I'm starting to work on deterministic output of modules. This is *really* important. Unfortunately, it's almost impossible to make progress as long as this code is actually firing because it kind of makes things definitively non-deterministic. ;]
> 
> Ben, do you have any concerns about only emitting the signature record for implicit modules? This would omit it for explicit modules (where I'm working), PCH files, etc. Based on your commit message and explanation, it doesn't really make sense outside of implicit modules AFAICT, and we're hoping it will go away completely.

SGTM although I would prefer we continue to check the /expected/ signature when an AST file imports an implicit module (and in particular when a PCH imports an implicit module).  Thanks for working on this!

> 
> 
> Also, I have one question here which may just be my ignorance:
> 
> On Thu, Oct 23, 2014 at 11:05 AM, Ben Langmuir <blangmuir at apple.com <mailto:blangmuir at apple.com>> wrote:
> +                            ASTFileSignature ExpectedSignature,
> +                            std::function<ASTFileSignature(llvm::BitstreamReader &)>
> +                                ReadSignature,
> 
> Wow, a std::function? Is this really necessary? Seems super heavy weight.

Oops, totally unnecessary.  Strength-reduced to a function pointer in r233050.

>  
> 
> +static ASTFileSignature readASTFileSignature(llvm::BitstreamReader &StreamFile){
> +  BitstreamCursor Stream(StreamFile);
> +  if (Stream.Read(8) != 'C' ||
> +      Stream.Read(8) != 'P' ||
> +      Stream.Read(8) != 'C' ||
> +      Stream.Read(8) != 'H') {
> +    return 0;
> +  }
> 
> This is truly magical. I assume this is checking for some magic string? Comments or something would be really nice here. Sharing the code would be even more nice.

Yeah, this is the AST/PCH file magic number.  Added “startsWithASTFileMagic” to factor this out of the three places we check this, also in r233050.

>  
> +
> +  // Scan for the CONTROL_BLOCK_ID block.
> +  if (SkipCursorToBlock(Stream, CONTROL_BLOCK_ID))
> +    return 0;
> 
> Why would it ever be reasonable to call this for a module file without such a block?

It’s not, but that’s exactly what returning 0 means - I added a doc comment to that effect.  Or was there something else here?

Thanks for the review,

Ben
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150323/af5d0815/attachment.html>