[PATCH] D144273: [clang][ASTImporter] Add VaList declaration to lookup table.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

@balazske

> I tried to find out how to add a correct test but could not check if this fails or not on AArch64 platform. [...] I want to touch ASTContext only if a test failure is found on AArch64 that makes it necessary.

It's possible to "simulate" the AArch64 behavior locally by temporarily modifying ASTContext.cpp to make `CreateX86_64ABIBuiltinVaListDecl` create the `std` namespace on x86_64 (using the code fragment taken from the AArch64-specific function). I tried to do this with a simple unit test, but my results are inconclusive (my test passed, but I'm not sure that it would've reproduced the error).

Consider using this hack to test the commit instead of relying on the centrally executed AArch64 tests.



================
Comment at: clang/test/Import/cxx-valist/Inputs/I1.cpp:1-3
+int *use_new(int N) {
+  return new int [N];
+}
----------------
I don't think that this //"import this innocent file before importing `int std`"// step is relevant for triggering the potentially problematic behavior that I outlined in my comments. Personally I'd remove this file from the commit (either keeping the the other two files as a simpler testcase that verifies that `int std` can be imported or converting that logic into a unit test); but I don't have a complete understanding of the situation, so if you feel that this is relevant, then I strongly support keeping it.

However, in that case I'd use a very simple "dummy" function like `int f(int x) { return x; }` instead of this `use_new` to keep this testcase independent of the //"does the type of `new` leak the declaration of the `std` namespace?"// question. (That was resolved in 2009 by commit [[ https://github.com/llvm/llvm-project/commit/87f540608106f28d9319b2148eb1b00192e020c2#diff-af1009156039bf1cca0adb74b8c38c7a4e9eacf7a570f1a47fd95261fe6dfc08 | 87f54060 ]] but a regression on that front could cause a misleading failure of this testcase.) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144273