@balazske , I tested these changes - again - and it all seems to work for me. I would have preferred we do not see the build status failures before doing this, but maybe we can be sure to avoid this next time (because I'll want to test again before our final merge). Please move ahead with your changes and let us know when you're ready for final review.

Hi @balazske . Can we go back to the original proposed fix and treat the new issue separately? We have an internal crash open that is corrected by the original patch I proposed, and passes all LITs and unit tests. I think it would be better to separate these concerns so we can move forward.
Thanks

Hi @balazske , all LIT and unittests pass with this change. By your logic, then we are missing some LIT or unittest cases that support your statement. Can you think of a case that demonstrates this? Thanks!

@balazske and I discussed, he will be commandeering this patch to improve.

LGTM

LGTM. Let's accept, merge and then watch to make sure we can keep the change.

LGTM

LGTM

Hi @donat.nagy , no problem. That's ok for me. Best!

Patch D144622 should be integrated into this one when a reduced reproducer has been prepared as a unittest and/or LIT case.

This patch needs a unit test (as @balazske mentioned). So far, the case we have is too large to be a suitable unittest or lit case - so requires reduction. @balazske , will you be adding this as a unittest or lit case?

I tested this change in the cases that previously failed for us, and this patch addresses our problems. It would be good to note in the commit header that this patch is intended to fix the problem first described by review D136886. Finally, this should be submitted as part of a patch stack intended to address the 2 AST importer problems seen and corrected, originating from D133468.

Changes are planned. Please do not waste any more time on this for now. This will probably be abandoned.

@donat.nagy , regarding the namespace leaking, there was a change -> https://reviews.llvm.org/D116774 that modified the behavior for aarch64 and arm. While not incorrect, it may offer some historical view or examples of how to address the current cases.

I got back to testing this through a large, internal set of builds, and do not see anymore problems. If everyone is ok with that, could this be merged?

rebase, bump review.

@DavidSpickett and/or @jrtc27 , I started with @mizvekov 's patch ( D136886) and attempted to address the problems with that patch on arm and aarch64. Is it possible for you to try testing this patch and/or commenting? Thank you. - Vince

Updates per suggestion from @balazske

@mizvekov, will you be picking this change up and finishing this, or do you mind if I have a go at finishing this patch? Thanks

I've made a very simple reproducer for this crash, clang-tidy executes on an x86 machine. Requires aarch64 and/or arm support compiled in.

I've reproduced the crash locally on an x86 machine. Working on a fix.

It appears to me this change https://reviews.llvm.org/D116774 is responsible for the unexpected behavior. Question for @jrtc27 : do you think if we could make this change consistent with https://reviews.llvm.org/D116774 that the problem would be addressed? Looks like we could make consistent changes in Sema.cpp and perhaps the problem would be addressed?

Thanks @Peter. I tried that, and the crash is resolved. Submitted https://reviews.llvm.org/D142627 for review.

Hello, it appears this patch causes a crash when I analyze this reproducer, with Z3 enabled. In the case shown here, the analyzer finds that 'f' to the call a(f) is uninitialized, and then is attempted to be refuted through SMTConv, leading to the assertion.

Hi @steakhal, thanks for the suggested change.
How we can help move this forward? From what I'm comprehending from the notes, perhaps we could try running this change through our internal systems level test and fuzzer. Unfortunately, I'd not be able to say more than "trust me, we saw no problems" if we see no problems. But if I do find additional cases I can make simplified reproducers and we can work on addressing those.
Best!

I think that addresses the last comments. Thanks again :)

clean up pass per comments from @steakhal

correct handling of sign type per @steakhal comments. Thank you, sir :)

Thanks Balazs, I think the comments have been addressed. Let me know if there's anything else to do, or if this is ready to land.

correct formatting of test case and expand test cases

Thanks for the comments. I assumed git-clang-format cleaned up the cruft, but it didn't - that's disappointing. I'll try these things and update the review. Best!

Some debug context ...

update commit header

@mizvekov, thanks for posting a fix. LGTM, but someone else must approve. Best!

Adding more information, seems this patch's "hack" returns the following

remove commented code from test case

remove commented line

update per comments from @martong

Thanks @martong , I understand. I'll make the changes and resubmit. Best!

a proposal to handle embedded null case caught by @steakhal

Thanks Balazs, you mean something like this correct?

I think all comments have been addressed, please let me know if I missed some detail :) Best!

address @steakhal comments

Use QualType::getAsString() per suggestion from martong

I was able to find and add a covering test case. I understand the fix may not be "correct", but it does avoid the crash demonstrated by the test case.

add test case

I know this will need a reproducer, and I'm working on that. That work is still in progress.

Thanks @martong. I fixed the "typo" :/, landing. Best!

fix a stupid leftover cut/paste mistake in the test case :/

Address comments from @martong

Thanks for the comments! Per our discussion, I'll create a different test case with a pinned target and add the additional test case to be sure we see expected behavior. Best!

polite ping to @NoQ and/or @martong. @steakhal is ok with this change, are you ok if I land this? Thanks!

update LIT per comments from @steakhal

clang-format

add back amdgcn cases

Hi @steakhal, I believe I've addressed the comments. Please have a look and let me know if this is what you were after. Best!

refactor patch to remove DefaultBool, replace with bool as suggested

I believe all comments have been addressed. After discussion with @steakhal, we're leaning towards removing the DefaultBool class and refactoring in favor of the language native bool type - mainly because DefaultBool doesn't add anything. Minimal yet effective is usually better.

Update test cases, address comments

The two small issues are addressed, I think the only outstanding issue is the question about the test case. Please let me know a preference and I'll implement accordingly. Best!

update per @steakhal's latest comments

Thanks for the the comments. I'll make the changes and resubmit. Please let me know your preferences for the test case (default vs pinned x86 or same).