<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Mar 23, 2015, at 10:40 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" class="">chandlerc@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="gmail_extra" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class="gmail_quote"><br class="Apple-interchange-newline">On Mon, Mar 23, 2015 at 10:37 PM, Ben Langmuir<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:blangmuir@apple.com" target="_blank" class="">blangmuir@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Mar 23, 2015, at 10:21 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank" class="">chandlerc@google.com</a>> wrote:</div><br class=""><div class=""><br class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; float: none; display: inline !important;" class="">On Mon, Mar 23, 2015 at 9:54 PM, Ben Langmuir<span class=""> </span></span><span dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;" class=""><<a href="mailto:blangmuir@apple.com" target="_blank" class="">blangmuir@apple.com</a>></span><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; float: none; display: inline !important;" class=""> </span><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; float: none; display: inline !important;" class="">wrote:</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;" class=""><blockquote class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><span class=""><blockquote type="cite" class=""><div class="">On Mar 23, 2015, at 5:38 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank" class="">chandlerc@google.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class="">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. ;]</div></div></blockquote><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""></div><div class="">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.</div></div></div></blockquote><div class=""><br class=""></div></span><div class="">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!</div></blockquote><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;" class=""> </div><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;" class="">I'm not planning to change the checking code path at all. All the tests pass if I just restrict the emission to be when generating implicit modules</div></div></blockquote><div class=""><br class=""></div></span><div class="">Perfect, that’s what I was hoping for. Sorry I didn’t phrase it clearly.</div></div><div class=""><div class="h5"><div class=""><br class=""><blockquote type="cite" class=""><div class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;" class="">Perhaps the confusion is that we have two totally different 'implicit/explicit' things in Modules land. There are implicit vs. explicit submodules. That's not what I'm talking about. I'm talking about whether -fno-implicit-modules is passed. When the build system is produces modules files for us, it doesn't seem like we ever need to take responsibility for the signature and can just omit it.</div><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;" class=""><br class=""></div><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;" class="">At least, all the regression tests pass when I make the above change, so even if it isn't the perfect state, I think it is a strict improvement. It at least allows me to test the removal of some other bit of non-determinism.</div><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;" class=""> </div><blockquote class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><span class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""></div><div class=""><br class=""></div><div class="">Also, I have one question here which may just be my ignorance:</div><div class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Thu, Oct 23, 2014 at 11:05 AM, Ben Langmuir<span class=""> </span><span dir="ltr" class=""><<a href="mailto:blangmuir@apple.com" target="_blank" class="">blangmuir@apple.com</a>></span><span class=""> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;">+ ASTFileSignature ExpectedSignature,<br class="">+ std::function<ASTFileSignature(llvm::BitstreamReader &)><br class="">+ ReadSignature,<br class=""></blockquote><div class=""><br class=""></div><div class="">Wow, a std::function? Is this really necessary? Seems super heavy weight.</div></div></div></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">Oops, totally unnecessary. Strength-reduced to a function pointer in r233050.</div><span class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><br class="">+static ASTFileSignature readASTFileSignature(llvm::BitstreamReader &StreamFile){<br class="">+ BitstreamCursor Stream(StreamFile);<br class="">+ if (Stream.Read(8) != 'C' ||<br class="">+ Stream.Read(8) != 'P' ||<br class="">+ Stream.Read(8) != 'C' ||<br class="">+ Stream.Read(8) != 'H') {<br class="">+ return 0;<br class="">+ }<br class=""></blockquote><div class=""><br class=""></div><div class="">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.</div></div></div></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">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.</div><span class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;">+<br class="">+ // Scan for the CONTROL_BLOCK_ID block.<br class="">+ if (SkipCursorToBlock(Stream, CONTROL_BLOCK_ID))<br class="">+ return 0;<br class=""></blockquote><div class=""><br class=""></div><div class="">Why would it ever be reasonable to call this for a module file without such a block?</div></div></div></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">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?</div></blockquote><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;" class=""><br class=""></div><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;" class="">If this is a programming invariant, it should be assert()-ed rather than returning something.</div></div></blockquote><br class=""></div></div></div><div class="">It’s an error condition not an invariant, since we’re reading a file provided by the user. The caller of this function produces a fatal error if it returns 0. Admittedly we aren’t very paranoid about malformed AST files in general, but an error seems better when we can trivially provide one.</div></blockquote></div></div><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">If we want to diagnose malformed AST files, we should be using a diagnostic, not a return of zero.</div></div></blockquote><br class=""></div><div>And the caller does emit a diagnostic. This could be tightened up by returning one of {found signature X, no signature found, malformed AST}. But I still think the latter cases are better as diagnosable errors than assertions.</div><div><br class=""></div><div><blockquote type="cite" class=""><div class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">The AST files are essentially programmatic interfaces between Clang and itself.</div></div></blockquote><blockquote type="cite" class=""><div class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""> I'm not aware of any realistic handling of malformed inputs like this. If we want to do this, we should do it correctly. If we're not actually doing it, it seems quite misleading to return a bizarre error number.</div></div></blockquote><div><br class=""></div><div>There are several ways for the user or their build system to break this interface in practice (e.g. user provides wrong kind of AST file; file written by different version of compiler; file truncated). We try to diagnose these cases.</div><div><br class=""></div><blockquote type="cite" class=""><div class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class=""></div><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">What prevents a random number of '0' from actually *being* the signature?</div></div></blockquote><div><br class=""></div><div>The code that produces the random value avoids 0 for this reason. If we went with what I said above, we could of course remove the sentinel.</div><br class=""><blockquote type="cite" class=""><div class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class=""></div><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">None of this seems really principled in its design, and as something that is in trunk for many months now, I think its principles need to be shored up.</div></div></blockquote></div></body></html>