[PATCH] D142822: [clang] ASTImporter: Fix importing of va_list types and declarations

[Whisperity via Phabricator via cfe-commits]

whisperity added subscribers: rsmith, whisperity.
whisperity added a comment.

In D142822#4095896 <https://reviews.llvm.org/D142822#4095896>, @DavidSpickett wrote:

>   /home/david.spickett/llvm-project/clang/test/CodeGenCXX/cxx20-module-std-subst-2b.cpp:14:18: error: reference to 'std' is ambiguous
>   void f(str<char, std::char_traits<char>> &s) {
>                    ^
>   /home/david.spickett/llvm-project/clang/test/CodeGenCXX/cxx20-module-std-subst-2b.cpp:7:11: note: candidate found by name lookup is 'std'
>   namespace std {
>             ^

This looks like an ouchie of the highest order. I've been looking into Modules... 5-ish years ago, when it was still under design. There are two major concerns here, one is that we've been having a feature called //"Modules"// which was very Clang-specific (but a good proof-of-concept spiritual predecessor of what became the Standard Modules), and there are the standard modules. Now the file name explicitly mentions `cxx20` so this is likely about Standard Modules stuff. But to be fair, I'd take this implementation with a pinch of salt. Due to the lack of build-system support for Modules (there are some murmurs these days on the CMake GitLab about very soon adding meaningful support for this...) we do not really seem to have large-scale real-world experience as to how the standard really could be made working, **especially** in such a weird case like touching `namespace std` which //might// not be standards-compliant in the first place. (But yeah, we are test code here in a compiler; we can cut ourselves some slack.)

The code itself in these test files is really weird; it seems the test is centred around the idea that there are:

  // TU "A"
  module; // Code in the *global module fragment*
  namespace std { /* ... */ class basic_string; }
  
  export module Whatever; // Code in the public module interface unit of module Whatever.
  export /* ... */ using str = std::basic_string<...>;

So we have a symbol called `str`, which is publicly visible (to TUs importing `Whatever`), and the `namespace std` preceding `Whatever` in `TU "A"` is **not** visible but **reachable**? (The standard's way of expressing reachability is a kind of a mess <http://eel.is/c++draft/module.reach>, so I'd **definitely** try finding the people who worked in developing the actual solution that makes Standard Modules //work// in Clang... call the help of someone who's really expert on the Standard...) Perhaps @rsmith could help us clarify this situation.

You can't name `std::basic_string<...>` in the client code (because it is not //visible//), but you can depend on the symbol (e.g., `decltype` it to an alias!) because it is //reachable//!

And then you have

  // TU "B"
  import Whatever;
  
  namespace std { /* ... */ struct char_traits {}; }
  
  // use Sb mangling, not Ss, as this is not global-module std::char_traits
  /* ... */
  void f(str<..., std::char_traits<...>>)

In which there is "another" (?) `namespace std`, which should(??) also be part of the global module <http://eel.is/c++draft/module#unit-6>:

> The //global module// is the collection of all //global-module-fragments// and all translation units that are not module units. Declarations appearing in such a context are said to be in the //purview// of the global module.

I believe that `TU "B"` is **not** a module unit... But the comment (which is somehow misformatted?) in it would like to say that this `std` is, in fact, //not// part of the global module.

So somehow, the changes in this patch are making //both// `std`s part of the same //purview// (whatever that means, really...), thus resulting in ambiguity.

- Does this only happen on non-X86? @vabridgers, you could try adding a `--target=` flag locally in the test file to the compiler invocation and re-running the tests to verify. (Might need an LLVM build that is capable of generating code of other architectures.)
- My other suggestion would be putting some `->dump()` calls in your change, running the code and observing how the ASTs are looking in the states when execution is within the functions you're trying to change.



================
Comment at: clang/test/AST/ast-dump-traits.cpp:55
 // CHECK-NEXT: |     `-ExpressionTraitExpr {{.*}} <col:10, col:28> 'bool' __is_lvalue_expr
-// CHECK-NEXT: `-FunctionDecl {{.*}} <line:30:1, line:35:1> line:30:6{{( imported)?}} test_unary_expr_or_type_trait 'void ()'
+// CHECK-NEXT:  -FunctionDecl {{.*}} <line:30:1, line:35:1> line:30:6{{( imported)?}} test_unary_expr_or_type_trait 'void ()'
 // CHECK-NEXT:   `-CompoundStmt {{.*}} <col:38, line:35:1>
----------------
How is this change legit? Is this `FunctionDecl` node just "floating"? If so, why is there a `-` preceding in the textual dump? Usually top-level nodes do not have such a prefix.


================
Comment at: clang/test/AST/fixed_point.c:405
 
-//CHECK-NEXT: `-VarDecl {{.*}} literallast '_Accum' cinit
+//CHECK-NEXT:  -VarDecl {{.*}} literallast '_Accum' cinit
 //CHECK-NEXT:   `-FixedPointLiteral {{.*}} '_Accum' 1.0
----------------
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142822