<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 4, 2015 at 2:18 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">On Thu, Jun 4, 2015 at 1:49 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div>On Thu, Jun 4, 2015 at 1:27 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div>On Wed, Jun 3, 2015 at 6:24 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div>On Wed, Jun 3, 2015 at 8:40 AM, Ben Langmuir <span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span> wrote:<br><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 style="word-wrap:break-word"><br><div><span><blockquote type="cite"><div>On Jun 2, 2015, at 7:57 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>> wrote:</div><br><div><div dir="ltr">Hi all,<div><br></div><div>Currently we do not seem to issue any form of diagnostic when there are missing headers named in a module map. See the attached test case. Even worse, we will just treat all headers in the module as textual. Hilarity ensues.</div></div></div></blockquote><div><br></div></span><div>Yep, this is <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_PR20507&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=CnzuN65ENJ1H9py9XLiRvC_UQz6u3oG6GUNn7_wosSM&m=7nONzHcaIDvfo55yS9XyE1yoEgranrBQF-0Ka132eHg&s=IYXezDA0p96a8zSVDcZl8sZdcl1s7yFc-UGGe4ibPbU&e=" target="_blank">llvm.org/PR20507</a></div><span><div><br></div><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>Some prior art in this area:<div>Daniel - r197485</div><div>Ben - r206664</div></div><div><br></div><div>Based on these commits, it ostensibly seems that clang does *some* sort of checking for missing files in a module map, but I can't seem to coax clang into doing this in C++ language mode.</div><div><br></div><div>Looking at the source code, it seems like we end up conflating "unavailable" due to a failed `requires` with "a header is missing". It sounds like we essentially need two notions "unsatisfied `requires`" and "necessary header is missing" (a header guarded by an unsatisfied `requires` does not count as "necessary"). Haven't dug in deep yet, but my hypothesis is that somewhere along the way we silently treat a module with missing headers as though it had an unsatisfied `requires`, leading to us silently neglecting that it was ever in a module at all.</div></div></div></blockquote><div><br></div></span><div>Yes, they both come out as the module being “unvavailable”, which is why we don’t import it. I think the right answer is that attempting to import any unavailable module (even via an auto-import) should be an error. That is:</div><div><br></div><div><div>module MissingHeader {</div><div> header “exists.h"</div><div> header “doesnt_exist.h”</div><div>}</div><div><br></div><div>#include <exists.h> // error, this would import MissingHeader, which is unavailable because we're missing header “doesnt_exist.h"</div></div><div><br></div><div>module Top {</div><div> header “Top.h”</div><div> module A {</div><div> requires non_existent</div><div> header “A.h”</div><div> }</div><div> module B {</div><div> header “B.h”</div><div> }</div><div>}</div><div><br></div><div>#include <Top.h> // OK</div><div>#include <A.h> // error, this would import Top.A, which is unavailable because of the missing requirment</div><div>#include <B.h> // OK</div><div><br></div><div>But handling the first case is more important I think.</div><span><br><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>I'm glad to put some effort into fixing this; I spent a good part of today with a bizarre error that I traced back to this and I don't want my users to have to deal with the same. Any pointers would be appreciated.</div></div></div></blockquote><div><br></div></span><div>That would be great! I was coincidentally hoping to fix this myself in the next few weeks, but if you beat me to it even better.</div><br></div></div></blockquote><div><br></div></div></div><div>I'm still acclimating myself to the modules code, do you have any concrete thoughts on how to do this? I was just planning on looking through for everywhere we use "unavailable" happens and figure out a pattern, but if you already have an idea, it might save me some time.</div></div></div></div></blockquote><div><br></div></div></div><div>Something like:</div><div> * make ModuleMap::findModuleForHeader able to produce an unavailable module, but treat it as being worse than any available module</div><div> * make Preprocessor::HandleIncludeDirective issue an error if its call to LookupFile produces a SuggestedModule that is unavailable</div><div><br></div><div>This should probably happen even if modules is disabled (that is, when we are parsing module maps but not actually building / using precompiled module files).</div></div></div></div></blockquote><div><br></div></div></div><div>Thanks for the pointers. I'll take a look at doing this.</div><span><div> </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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><div><br></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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Also, any preferred bikeshed for the names for "unvailable because of unsatisfied requires" and "necessary header is missing" concepts in the source?</div></div></div></div></blockquote><div><br></div></span><div>If the response to both is the same (as Ben suggested, and I agree) then I'm not sure that we need to distinguish them anywhere other than the place where we produce the diagnostic.</div></div></div></div></blockquote><div><br></div></span><div>But aren't they different when it comes time for clang to build the module? We want to build a top-level module even if it has a submodule with an unsatisfied "requires" (e.g. _Builtin_intrinsics has submodules with contradictory `requires`), taking appropriate action to exclude any headers with unsatisfied `requires`. On the other hand (modulo submodules already removed from consideration due to unsatisfied `requires`), a missing header should be treated as a hard error.</div></div></div></div></blockquote><div><br></div></div></div><div>I think you're right, but I think that will be the emergent behavior from treating the two cases the same: if a module's header is missing, the corresponding *top-level* module (and all its submodules) gets marked unavailable, so we should never try to implicitly build it.</div></div></div></div></blockquote><div><br></div><div>Thanks for the explanation, that makes perfect sense.</div><div><br></div><div>-- Sean Silva</div><div> </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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>There might be a file system race here, where the main build process thinks the module is available, but the module build finds a header is missing and builds an empty module because all the submodules are unavailable. That's probably worth fixing, but again the fix doesn't really depend on the different kinds of availability: if the top-level module is unavailable, we should produce an error if we try to build a module file for it. (This is probably easy to test by building an unavailable module with -cc1 -emit-module.)</div></div></div></div>
</blockquote></div><br></div></div>