Decide on how to handle differences in configurations between module dependencies

[matts1]

This is a follow-up to #131569. Please read that PR for context

@Bigcheese @ChuanqiXu9 @cor3ntin @mizvekov @atetubou FYI

The current problem is that there are several options such as RTTI and exceptions that affect the codegen phase, and not the generation of the AST. However, they unfortunately are visible through macros, and thus can indirectly affect the generation of the AST.

I have created a demo at https://github.com/matts1/demo/tree/module-config-mismatch which you're welcome to experiment and play around with. This is my summary of my conclusions from the demo:

Likely changes based on macros

• Most of the time it will be perfectly safe, as header files are unaffected (example)
• Sometimes dependent macros will be created that can be observed (eg. LIB_HAS_EXCEPTIONS)
• Sometimes methods may only appear (example) when a macro is / isn't defined.
• It is possible to have a scenario where the signature changes based on whether a macro is defined, though in practice I haven't seen any instance of this
  • Eg. void foo() for exceptions vs Error foo() for no-exceptions

Without modules

If user.cc is built without exceptions, it naturally includes lib.h without exceptions, and thus it retrieves the version of the header file for no exceptions. Thus, linking against the version of lib.o built with exceptions may not work if the header files differ in a way that affects the API, or if you attempt to read macros that were based on lib's __cpp_exceptions flag.

Thus, unless lib.h is agnostic to whether a given macro is defined, it does not make sense to be able to build lib with a macro defined, and the dependent library without.

With modules

With modules, if a user of a library defines a macro, it is not visible to said library. Thus, it becomes possible to compile a library with different parameters to its users (on a purely technical level). However, whether we should do it is another question. So let's go through the demo.

Note that we need to decide on the semantics both with and without the -W[no-]module-file-config-mismatch flags.

-Wmodule-file-config-mismatch

Current behaviour:

clang++ -cc1 -x c++ -emit-module -o out/modules/user_except_lib_noexcept.pcm -fmodules modules.modulemap -fmodule-name=user -fexceptions -fcxx-exceptions -fmodule-file=out/modules/lib_noexcept.pcm
error: exception handling was disabled in PCH file but is currently enabled
error: module file out/modules/lib_noexcept.pcm cannot be loaded due to a configuration mismatch with the current compilation [-Wmodule-file-config-mismatch]
2 errors generated.

I believe that the current behavior is the correct one here. While, as I said, it's technically possible to have one library depend on another with different configuration, it is a rare situation that should be used with an abundance of caution.

-Wno-module-file-config-mismatch

Is it reasonable to depend on a mismatch?

I think my library demonstrates a reasonable use case for not just why we should allow depending across changed langopts, but also why things being observable through macros is the behaviour we want.

• The library changes header file contents based on whether exceptions are enabled for said library
• The library exposes this change through the macro LIB_HAS_EXCEPTIONS
• The user can read LIB_HAS_EXCEPTIONS to determine whether the dependent library was built with exceptions.

Current behaviour:

In summary, when there is a config mismatch, it will create a new module for the current configuration and use that. It will not truly allow a config mismatch.

clang++ -cc1 -x c++ -emit-module -o out/modules/user_except_lib_noexcept.pcm -fmodules modules.modulemap -fmodule-name=user -fexceptions -fcxx-exceptions -fmodule-file=out/modules/lib_noexcept.pcm -Wno-module-file-config-mismatch

The above command succeeds, but not in a way I believe to be correct.

1. It observes that the user module is attempting to depend on the lib module
2. It finds all instances of the lib module, and gets one with exceptions disabled.
3. It finds that there is no configuration matching, and that we have -Wno-module-config-mismatch
4. It attempts to read the header file in a manner similar to no modules (ie. with the same header configuration)
5. It has now successfully compiled, but what it did was to silently compile against a version of lib which has exceptions

Proposed behaviour:

As such, I propose the following change:

• (unchanged) LANGOPTs cannot differ between a library and its user
• (unchanged) BENIGN_LANGOPTs don't affect the construction of the AST, and can thus differ safely
• (unchanged) COMPATIBLE_LANGOPTs affect the construction of the AST in a compatible way, and can thus differ safely
• (new) MOSTLY_COMPATIBLE_LANGOPTs (not satisfied with the name - suggestions welcome) do not directly affect the AST in incompatible ways, but are observable through the preprocessor, and thus may be different in arbitrary ways. When the flag "treat mostly compatible as compatible" is set, these are treated as compatible. While this is not a guarantee of safety, this should generally work under any of the following conditions:
  • The library API does not change based on the macro (this should be true most of the time)
  • The library API does change based on the macro, but exposes a macro of its own to check whether it's enabled, and the user checks that macro when using parts of the library that change instead of the one for the option directly (eg. check LIB_HAS_EXCEPTIONS instead of __cpp_exceptions)

The reason I suggest a new flag rather than using the existing -Wno-module-file-config-mismatch is because of backwards compatibility - there may be some people who rely upon the existing behaviour. If this isn't something we're concerned about, I'm happy to just use that existing flag.

[llvmbot]

@llvm/issue-subscribers-clang-modules

Author: Matt (matts1)

This is a follow-up to #131569. Please read that PR for context

@Bigcheese @ChuanqiXu9 @cor3ntin @mizvekov @atetubou FYI

The current problem is that there are several options such as RTTI and exceptions that affect the codegen phase, and not the generation of the AST. However, they unfortunately are visible through macros, and thus can indirectly affect the generation of the AST.

I have created a demo at https://github.com/matts1/demo/tree/module-config-mismatch which you're welcome to experiment and play around with. This is my summary of my conclusions from the demo:

Likely changes based on macros

• Most of the time it will be perfectly safe, as header files are unaffected (example)
• Sometimes dependent macros will be created that can be observed (eg. LIB_HAS_EXCEPTIONS)
• Sometimes methods may only appear (example) when a macro is / isn't defined.
• It is possible to have a scenario where the signature changes based on whether a macro is defined, though in practice I haven't seen any instance of this
  • Eg. void foo() for exceptions vs Error foo() for no-exceptions

Without modules

If user.cc is built without exceptions, it naturally includes lib.h without exceptions, and thus it retrieves the version of the header file for no exceptions. Thus, linking against the version of lib.o built with exceptions may not work if the header files differ in a way that affects the API, or if you attempt to read macros that were based on lib's __cpp_exceptions flag.

Thus, unless lib.h is agnostic to whether a given macro is defined, it does not make sense to be able to build lib with a macro defined, and the dependent library without.

With modules

With modules, if a user of a library defines a macro, it is not visible to said library. Thus, it becomes possible to compile a library with different parameters to its users (on a purely technical level). However, whether we should do it is another question. So let's go through the demo.

Note that we need to decide on the semantics both with and without the -W[no-]module-file-config-mismatch flags.

-Wmodule-file-config-mismatch

Current behaviour:

clang++ -cc1 -x c++ -emit-module -o out/modules/user_except_lib_noexcept.pcm -fmodules modules.modulemap -fmodule-name=user -fexceptions -fcxx-exceptions -fmodule-file=out/modules/lib_noexcept.pcm
error: exception handling was disabled in PCH file but is currently enabled
error: module file out/modules/lib_noexcept.pcm cannot be loaded due to a configuration mismatch with the current compilation [-Wmodule-file-config-mismatch]
2 errors generated.

I believe that the current behavior is the correct one here. While, as I said, it's technically possible to have one library depend on another with different configuration, it is a rare situation that should be used with an abundance of caution.

-Wno-module-file-config-mismatch

Is it reasonable to depend on a mismatch?

I think my library demonstrates a reasonable use case for not just why we should allow depending across changed langopts, but also why things being observable through macros is the behaviour we want.

• The library changes header file contents based on whether exceptions are enabled for said library
• The library exposes this change through the macro LIB_HAS_EXCEPTIONS
• The user can read LIB_HAS_EXCEPTIONS to determine whether the dependent library was built with exceptions.

Current behaviour:

In summary, when there is a config mismatch, it will create a new module for the current configuration and use that. It will not truly allow a config mismatch.

clang++ -cc1 -x c++ -emit-module -o out/modules/user_except_lib_noexcept.pcm -fmodules modules.modulemap -fmodule-name=user -fexceptions -fcxx-exceptions -fmodule-file=out/modules/lib_noexcept.pcm -Wno-module-file-config-mismatch

The above command succeeds, but not in a way I believe to be correct.

1. It observes that the user module is attempting to depend on the lib module
2. It finds all instances of the lib module, and gets one with exceptions disabled.
3. It finds that there is no configuration matching, and that we have -Wno-module-config-mismatch
4. It attempts to read the header file in a manner similar to no modules (ie. with the same header configuration)
5. It has now successfully compiled, but what it did was to silently compile against a version of lib which has exceptions

Proposed behaviour:

As such, I propose the following change:

• (unchanged) LANGOPTs cannot differ between a library and its user
• (unchanged) BENIGN_LANGOPTs don't affect the construction of the AST, and can thus differ safely
• (unchanged) COMPATIBLE_LANGOPTs affect the construction of the AST in a compatible way, and can thus differ safely
• (new) MOSTLY_COMPATIBLE_LANGOPTs (not satisfied with the name - suggestions welcome) do not directly affect the AST in incompatible ways, but are observable through the preprocessor, and thus may be different in arbitrary ways. When the flag "treat mostly compatible as compatible" is set, these are treated as compatible. While this is not a guarantee of safety, this should generally work under any of the following conditions:
  • The library API does not change based on the macro (this should be true most of the time)
  • The library API does change based on the macro, but exposes a macro of its own to check whether it's enabled, and the user checks that macro when using parts of the library that change instead of the one for the option directly (eg. check LIB_HAS_EXCEPTIONS instead of __cpp_exceptions)

The reason I suggest a new flag rather than using the existing -Wno-module-file-config-mismatch is because of backwards compatibility - there may be some people who rely upon the existing behaviour. If this isn't something we're concerned about, I'm happy to just use that existing flag.

[ChuanqiXu9]

My question is, if we introduce a new language option catagory, who is going to mark the existing lang options correctly? Note that the existing lang options are not marked precisely. A lot of them are marked as LANGOPT by default.

[matts1]

While it's a reasonable question, I don't think it's any different to the current situation. I personally took issue with exceptions being a standard langopt, and so I had to file an issue / PR to fix it. I think the only difference would be that now, since exceptions doesn't fit into either benign or compatible, it fits into this new category.

In other words, just like the current situation, someone will have to go and manually change it. I think that it's kind of annoying, but I don't see any solution where that could be avoided.

[ChuanqiXu9]

While it's a reasonable question, I don't think it's any different to the current situation. I personally took issue with exceptions being a standard langopt, and so I had to file an issue / PR to fix it. I think the only difference would be that now, since exceptions doesn't fit into either benign or compatible, it fits into this new category.

In other words, just like the current situation, someone will have to go and manually change it. I think that it's kind of annoying, but I don't see any solution where that could be avoided.

I don't mind it. I just want to make sure I understand the idea better.

Then if I read correctly, MOSTLY_COMPATIBLE_LANGOPT will act like COMPATIBLE_LANGOPT with an option but as LANGOPT without the corresponding option.

[matts1]

Then if I read correctly, MOSTLY_COMPATIBLE_LANGOPT will act like COMPATIBLE_LANGOPT with an option but as LANGOPT without the corresponding option.

That's a very good summary, yes.

[ChuanqiXu9]

I don't feel bad for this. Minor improvements are always welcome.