[PATCH] D141450: [Clang][cc1] Add -fno-modules-local-submodule-visibility to override the default

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D141450#4052210 <https://reviews.llvm.org/D141450#4052210>, @jansvoboda11 wrote:

> In D141450#4049049 <https://reviews.llvm.org/D141450#4049049>, @dblaikie wrote:
>
>> Perhaps I've got things confused, but my understanding of LSV was that it prevented other headers in the same modulemap from leaking into the use/inclusion of one header in a module. But that indirect inclusions were still valid/exposed (eg: header A, B, and C in a module, A includes C - without LSV, including A also incidentally exposes B, but with or without LSV including A does expose C).
>
> Regardless of LSV, importing A doesn't incidentally expose B. For exposing C, module A would need to re-export C.
>
>> My understanding was that LSV wouldn't reject anything that non-modular builds accepted,
>
> LSV rejects things that both non-LSV modular and textual builds accept:
>
>   // RUN: rm -rf %t
>   // RUN: split-file %s %t
>   
>   //--- module.modulemap
>   module M {
>     module S1 { header "s1.h" }
>     module S2 { header "s2.h" }
>   }
>   
>   //--- s1.h
>   #define T1 int
>   
>   //--- s2.h
>   T1 foo();
>   
>   //--- tu.c
>   #include "s1.h"
>   #include "s2.h"
>   
>   // Let's see how "T1" becomes available in "s2.h" with different configurations.
>   
>   // Textual build: succeeds, "tu.c" includes "s1.h" before "s2.h".
>   // RUN: %clang_cc1 -fsyntax-only %t/tu.c
>   
>   // Modular build: succeeds, the entire compilation of M shares one preprocessing context,
>   //                which includes "s1.h" before "s2.h" due to their order in the module map.
>   // RUN: %clang_cc1 -fsyntax-only %t/tu.c -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache
>   
>   // Modular build with LSV: fails, compilation of M creates separate context for each submodule,
>   //                         so "s2.h" does not see symbols of "s1.h" (without explicitly importing it).
>   // RUN: %clang_cc1 -fsyntax-only %t/tu.c -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -fmodules-local-submodule-visibility

Ah, I guess that's sort of incidental, though - the non-modular code is brittle (has a header order dependency) & would be made non-brittle by including "s1.h" from "s2.h"

but yeah, fair point - `-fmodules-local-submodule-visibility` does cause false negatives, in the sense that code that compiled without modules would be rejected with modules.

>> but would prevent modules from accepting things that non-modular builds rejects. (so LSV is a way to remove modular build false-accepts, basically)
>
> This is true:
>
>   ...
>   
>   //--- tu.c
>   #include "s2.h"
>   
>   // Textual build: fails, "s2.h" has no way of knowing about "T1".
>   // RUN: %clang_cc1 -fsyntax-only %t/tu.c
>   
>   // Modular build: succeeds, "T1" is available in "s2.h" as a leftover from compiling "s1.h".
>   // RUN: %clang_cc1 -fsyntax-only %t/tu.c -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache
>   
>   // Modular build with LSV: fails, "T1" is not available in the preprocessing context of "s2.h".
>   // RUN: %clang_cc1 -fsyntax-only %t/tu.c -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -fmodules-local-submodule-visibility

Right right

>> Bugs/incompleteness with LSV+ObjC I could believe/understand, and if that's the main issue I guess that's a choice that can be made about which bugs are worth fixing, etc, and maybe we should document the LSV change as working around those bugs for now (perhaps forever - if the benefits of LSV aren't worth the investment in fixing those bugs).
>
> So yeah, we probably have some bugs/incompleteness specific to LSV+ObjC, but our SDK also contains code that compiles fine with both textual inclusion and modules, but fails with modules + LSV. Having a way to disable LSV under Objective-C++20 is necessary for us in that case. Note that this patch doesn't change any behavior yet, it just makes `-fno-modules-local-submodule-visibility` available in `-cc1`.

*nod* Thanks for walking me through it though. I wasn't aware of the rejects-'valid' case. Makes sense that some legacy codebase needs this feature to continue to build. I was only aware of/thinking of the second case where non-LSV would accept-invalid & that would imply a codebase that could /only/ compile with modules, which I was surprised by - I understand my misunderstanding now.

Carry on!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141450/new/

https://reviews.llvm.org/D141450