[clang] [clang][modules] Headers meant to be included multiple times can be completely invisible in clang module builds (PR #83660)

James Y Knight via cfe-commits cfe-commits at lists.llvm.org
Fri Apr 12 14:13:12 PDT 2024


jyknight wrote:

I think the bug this change was attempting to fix is actually the same as #38554 and related bugs -- which, it appears, was not really fixed.

The underlying problem here is that `#pragma once` should should work identically to ifndef guards, as far as what macros/decls are made visible to the includer. However, with the current modules implementation, it fails to do so. This PR addresses only one _symptom_ of that issue, but does not address the underlying problem.

Additionally, I think the semantics change implemented here is a bad idea. It significantly complicates the semantics of `#import`, in a way that I think is justifiable, especially given that there is a way to fix the identified issue in a more principled way.

Yes, `#import` was a bad and confusing idea, and should have been dropped 20 years ago, but adding more special cases to it does not make it better. A header file being marked "textual" in a module-map does _not_ mean a file should be be included multiple times -- it simply means that you didn't want to build it into a module. Linking these two concepts together ties together concepts which have no 

If this PR was the only way to fix the identified issue, maybe it could via an assumption that the current buggy `#pragma once` behavior is expected behavior which should be preserved.

As such, I would propose this PR ought to be reverted, and work to fix the underlying issue be done instead.

Here's a test case which I think demonstrates the actual bug -- it fails both before and after this PR. If you modify 'header.h' to use include guards, then it passes. This difference in behavior should not exist.
```
// RUN: rm -rf %t
// RUN: split-file %s %t
// RUN: %clang_cc1 -std=c17 -fmodules-cache-path=%t/no-lsv -fmodules -fimplicit-module-maps -I%t %t/pragma-once-modules.c -verify
// RUN: %clang_cc1 -std=c17 -fmodules-cache-path=%t/lsv -fmodules -fimplicit-module-maps -fmodules-local-submodule-visibility -I%t %t/pragma-once-modules.c -verify

//--- pragma-once-modules.c
// expected-no-diagnostics
#include "one.h"
#include "header.h"
my_int test(void) {
  return MY_MACRO;
}

//--- module.modulemap
module Submodules {
  module one {
    header "one.h"
    export *
  }
  module two {
    header "two.h"
    export *
  }
}

//--- one.h
#ifndef one_h
#define one_h
#endif

//--- two.h
#ifndef two_h
#define two_h
#include "header.h"
#endif

//--- header.h
#pragma once
#define MY_MACRO 5
typedef int my_int;
```

It produces the following unexpected output.
```
error: 'expected-error' diagnostics seen but not expected: 
  [...]/pragma-once-modules.c Line 4: missing '#include "header.h"'; 'my_int' must be declared before it is used
  [...]/pragma-once-modules.c Line 5: use of undeclared identifier 'MY_MACRO'
error: 'expected-note' diagnostics seen but not expected: 
  [...]/header.h Line 3: declaration here is not visible
```

https://github.com/llvm/llvm-project/pull/83660


More information about the cfe-commits mailing list