In D99714#2663677, @balazske wrote:

It works not reliable for all data types. If char is used instead of int (in the test), the allocated size may be larger than the intended size of the array, probably because memory alignment adjustments. In the following case it is possible to index "past the end" of the array for some first indices (until 12?).

struct S {
  int n;
  char x;
  char s[];
};

The tests are really promising! :)

I like it, looks good to me!

LG!

Ah, I see you need nonloc::SymbolVal in your next patch, and getDynamicSizeWithOffset returns an Unknown if the extend is symbolic.

I went through the change and it looks good, seems like this is indeed a copy paste error from line 132.
I checked the related conversation, and thanks for all the effort spent with the test.

LGTM! Thanks!

• Update comment

Thanks for the review guys! :)

• Add check for specialization kind

• Fix typo

• Remove handling of assignment

So something like an ASTConverter that just sits on top of the ASTImporter? In theory that class could implement the ImportImpl callback and do the custom imports it needs to do on its own

I am seeing some difficulties with this approach b/c this would require to replace an existing node in the AST with a new one in some cases. And that might not be always possible without a full retraverse of the AST. Perhaps we could come up with some simplified visitation or could reuse some data from ASTImporter::ImportedDecls.

Hi Raphael,

• Rename test case name to something meaningful

Guys, thanks for the review! :)

• Use isZeroConstant()
• Make trackRValueExpression private (static function)
• Fix clang-tidy report: auto *BO ---> const auto *BO

• Assert in the callee and check in the caller
• Fix typo in comment

• Rebase

Thanks for the review!

Sounds good to me!

LGTM! Thanks!

LG! Thanks!

Hi @aaron.ballman @riccibruno @dblaikie, I am adding you as a reviewer because it seems you have great experience with Clang's AST.
Could you please take a look on this patch? The problem is that we currently do not handle a CallExpr's return type properly in case of a dependent context. This seems to be a flaw in the implementation of CallExpr.

Good catch! Thank you!

This causes a crash if the function is called in such case.

Thanks for the test!

Thanks for the review!

Add a test case which fails if lit --vg is used.

In D97849#2600300, @teemperor wrote:

We can (and do) run LIT tests under valgrind, so if you have a test that triggers this valgrind error then that seems like the way to go. I don't think the test has to deterministically fail outside valgrind to be a valid test.

In D97849#2600220, @steakhal wrote:

In D97849#2600201, @aaron.ballman wrote:

This looks reasonable to me (good catch!), but is there a way for us to add a regression test for it?

I'm not sure if it's possible to write a test for deterministically demonstrating the bug - which is a non-deterministic crash.
So, even if we would have a test case, that would not catch the regression deterministically.
We could include the minimal reproducer for CTU analysis - the way we observed and tracked down this crash.

AFAIK, it did not reproduce with memory sanitizers.

Still looks good.
(The parent is an NFC and only this patch changes the behavior (and only if the cmdline flag is there), right? So, I'd expect the big impact from this patch.)

Still looks good to me! Thanks for handling the pragma cases!

Adding reviewers.
@rsmith as code owner,
@majnemer based on git blame of CallExpr::getCallReturnType.

Hi Balázs,

In D69726#2471657, @vabridgers wrote:

I was referred to this patch from https://reviews.llvm.org/D86743. I pulled this patch under review, brought it up to date and pushed to github at https://github.com/vabridgers/llvm-project-dev.git, branch: vla-fam-fixes. Everything seems ok on this branch (LITs pass, reproducers from https://bugs.llvm.org/show_bug.cgi?id=47272 and https://bugs.llvm.org/show_bug.cgi?id=28450 no longer crash). I can continue and push a change to Phabricator for review, or @Charusso and/or @balazske could finish this? I didn't want to just push an update without asking first :/ Cheers!

Thanks for the update!

In D93224#2497632, @steakhal wrote:

It seems quite a challenge to hook the Preprocessor for all possible configurations for every CompilerInvocation.
The underlying machinery is somewhat complex and spaghetti to me.

Here is what I suggest:
For now, this expansion is better than the previous was. Macro expansions will work for the main TU even in any CTU configuration. For the imported TUs, it will just not expand any macros.
This is a regression in some way, but we should live with that until we implement it completely.
I think the users would prefer a non-crashing partial implementation to a crashing one.

If you think it's an appropriate, I will update the CTU tests to expect no macro expansion in any imported TU.
And also remove the XFAIL.
This way we could still target clang-12.

Do you think it's acceptable?
@xazax.hun @NoQ

Nice work! Thanks!

Looks good to me! Thanks!

In D92797#2505254, @compnerd wrote:

Ping x 3

@balazske
Thanks Balázs! Great work!

Thanks for your work, I am glad that finally the Window bug is fixed with a simple solution.

Looks quite straight-forward other than that new prototype.

Nice and precise work! And special thanks for the unit tests. I've found some minor nits.

In D92797#2452497, @compnerd wrote:

Ping

In D92634#2453822, @OikawaKirie wrote:

I think it could be better to implement this check with a checker on PreStmt<BinaryOperator> and so on. And IMO, checkers have enough functionalities to report these problems.

Besides, the return value should be the exact value computed from the two integers, even unknown, rather than undefined. As the developers may overflow an integer on purpose.

In D92634#2451646, @danielmarjamaki wrote:

No reviews => I will not contribute.

Shafik, thanks for the review!

Thanks for the update. I checked it, still looks good to me.

• Check the attribute in the test as well

I wouldn't worry much about the ASTMerge tests, those are mostly legacy and not actively used nowadays. (At one point we were thinking even to remove them because we could got rid of the ASTMergeAction which is exclusively used for testing.)
The main test infrastructure for the ASTImporter is in the unittests. Those tests are exercised even with -fdelayed-template-parsing and with -fms-compatibility. So I am fine with marking the ASTMerge tests with XFAIL on windows.

LGTM!

• Remove not relevant param from test

However, every single TU in LLDB starts being pretty much empty (we do have some dummy declarations in them, but none of them have any name conflicts with declarations in any other AST we import from). So technically the 'empty' LookupTable is accurate.

In D92848#2442609, @martong wrote:

In D92848#2440205, @teemperor wrote:

So the only downside I can see is that the ASTImporter is no longer considering non-imported declarations in the To context (which are not known to the SharedState) for merging?

This is the case if you create the SharedState without a lookup table. If you pass a TUDecl then a lookup table will be created and in the constructor of that we traverse the AST to initialize the lookup. And from that point we will find every existing Decl even if they were not imported. As it is now we'd still be using the old noload_lookup.

@martong I assume I can make a follow-up for removing all the ASTImporter code for situations without a shared state?

Yes, that'd would be super.
Other than that, if you could make sure that the ASTImporterLookupTable is initialized then noload_lookup would not be needed and we could simplify so much in clang::ASTImporter.

In D92848#2440205, @teemperor wrote:

So the only downside I can see is that the ASTImporter is no longer considering non-imported declarations in the To context (which are not known to the SharedState) for merging?

This is the case if you create the SharedState without a lookup table. If you pass a TUDecl then a lookup table will be created and in the constructor of that we traverse the AST to initialize the lookup. And from that point we will find every existing Decl even if they were not imported. As it is now we'd still be using the old noload_lookup.

Thanks for the review guys!

• Remove if (!DC || !LexicalDC)

Thanks for the review!