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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

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



> 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



> 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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141450