In D98632#2626334, @aaron.ballman wrote:

LGTM! The CI build failures look to be entirely unrelated to any changes in this patch, but please keep an eye on the build bots once you land this just in case there's a real issue.

@aaron.ballman Thanks for that. Let's see if this one will work.

Let's see if this time it worked. Thanks.

In D98204#2617636, @aaron.ballman wrote:

The changes look good as far as the diff is concerned, but the patch does not apply cleanly for me on ToT and the CI pipeline agrees (https://buildkite.com/llvm-project/diff-checks/builds/32992#831d7148-ce1c-4e47-9f96-e2d7edf67580).

In D95462#2523435, @bader wrote:

LGTM. Let me know if you need help with submitting this patch.

rG37340798ccb0

I am not really convinced that this is an openmp issue. If I take the same test case but remove one level of instantiation, the test cases compiles without hitting the assertion.
template <typename T>
struct z {

static void aj() {
  T f;

#pragma omp target map(f)

  ;
}

};
template <int> struct as {};
template class z<as<4>>;
I think the issue is that Sema::FindAssociatedClassesAndNamespaces is entered with a null location and noone inside addAssociatedClassesAndNamespaces updates the field Instantiation of Result.
I am proposing this fix to remedy to it. Not sure what you think of this?
Thanks.

In D91089#2385821, @hans wrote:

clang.exe -c test.cpp
Assertion failed: !hasUninstantiatedDefaultArg() && "Default argument is not yet instantiated!", file D:\IUSERS\zahiraam\llorg_ws\ws1\llvm\clang\lib\AST\Decl.cpp, line 2719
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.

Thanks for catching that!

What's happening is that Clang tries to emit the ctor closure even though the ctor is just declared, but not defined.

I don't think it makes sense to try to emit the closure until we have the ctor definition. I'll update the patch to handle this.

In D91089#2383433, @hans wrote:

Please take a look.

In D90943#2379563, @rnk wrote:

It seems like OpenMP can trigger template instantiation without providing a location for the instantiation, and that is a bug. See this commented out assert in RequireCompleteTypeImpl:
https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaType.cpp#L8553

// FIXME: Add this assertion to make sure we always get instantiation points.
//  assert(!Loc.isInvalid() && "Invalid location in RequireCompleteType");

I think the workaround is probably OK, but if it's possible to loop at the OpenMP sema side of things, it's worth looking into.

Merged in https://reviews.llvm.org/rGe562a40871da.

Any feedback please?

In D88112#2298597, @rnk wrote:

I don't feel like there is enough information in the bug or here to understand if this is the correct fix. Why is clang crashing? Why should we return null here instead of returning a decl that has already been marked invalid? Maybe the caller should treat the invalid declaration more carefully. I usually try to answer these questions by reading the existing code to try to understand why it does what it does.

I see there are two call sites to this function, one for definitions ({}) and one for declarations {;}:
https://github.com/llvm/llvm-project/blob/master/clang/lib/Parse/ParseCXXInlineMethods.cpp#L39
https://github.com/llvm/llvm-project/blob/master/clang/lib/Parse/ParseDeclCXX.cpp#L2825

In this case, it seems like the second is the one that is crashing. Reading forward from that function call, I found these ifs here, which treat null and invalid member declarations differently:
https://github.com/llvm/llvm-project/blob/master/clang/lib/Parse/ParseDeclCXX.cpp#L2890
That means there is some set of things we do for invalid decls, and some that we do for null decls. I guess the question is now, should those things be done for these types of friend declarations? I guess the important thing is that the invalid declaration is added to the declgroup, which is ultimately what gets returned out of here, so maybe the crash happens later, but I can't say for sure.

In D88150#2298923, @rnk wrote:

Thanks!

Review anyone please? Thanks.

This patch is only to fix the compiler crash.
Would you mind commenting/reviewing this patch as it is a crash with (internal) impact?
There would still be another bug that needs fixing (would prefer to open another bug for it), for this case:
class __declspec(dllexport) foo {

foo(int x = 0);

};

In D84038#2179734, @mehdi_amini wrote:

In D84038#2179551, @zahiraam wrote:

I actually made an attempt to commit the patch using arc but not sure it made it. This was my last command: git commit --amend -a
Do I need to push now?
Thanks.

You can use arc land --revision D84038 or you can check what you're about to push: git fetch origin && git log ^origin/master HEAD ; if this is the commit you intend to push and the commit message is satisfying, then just push it.

If you don't have push permissions, you need to ask here for someone to land it for you.

I actually made an attempt to commit the patch using arc but not sure it made it. This was my last command: git commit --amend -a
Do I need to push now?
Thanks.

Can someone submit it please? Thanks.

Review please? Thanks.

In D84038#2165110, @beanz wrote:

Sadly I think @rnk isn't around. This is something I'd really like him to weigh in on. I assume there was a reason for doing this per-object file rather than for everything, and I don't have the context.

@hans Thanks for the feedback.
@riccibruno Thanks for the feedback.

In D79591#2092223, @hans wrote:

I'm sorry, but I still don't understand, and neither will anyone who reads the new code, I think. It needs an explanation.

@hans Thanks for looking at it. I think the updated patch is a better way to handle the crash.

@majnemer Any feedback? Thanks.

In D70691#1775169, @erichkeane wrote:

I looked at the test failure, the REQUIRES line is necessary because the test calls emit-obj. I've asked Zahira to commit it to fix the bots.

Got other build fails.
Doesn't the LIT test require a:
// REQUIRES: x86-registered-target
command?

In D70691#1773611, @leonardchan wrote:

LGTM. Could you commit this soon to get the bots green again?

This might be easier to read the edit that I have made:
ksh-3.2$ git diff
diff --git a/clang/test/CodeGen/opt-record-1.c b/clang/test/CodeGen/opt-record-1.c
index 3f37e32..00a753d 100644

• a/clang/test/CodeGen/opt-record-1.c

+++ b/clang/test/CodeGen/opt-record-1.c
@@ -1,12 +1,12 @@
- RUN: %clang_cc1 %s -opt-record-file=t1.opt -fopenmp -emit-llvm-bc -o %t.bc
- RUN: %clang_cc1 -x ir %t.bc -opt-record-file %t.opt -fopenmp -emit-obj
+ RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -target-cpu x86-64 %s -O3 -opt-record-file=t1.opt -fopenmp -emit-llvm-bc -o %t.bc
+ RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -target-cpu x86-64 -O3 -x ir %t.bc -opt-record-file %t.opt -fopenmp -emit-obj
// RUN: cat %t.opt | FileCheck -check-prefix=CHECK %s

If you agree with the edit I will commit the change. Thanks.

Please see the edit in the list test. Fixed the build issue that occurred in the buildbot:
http://lab.llvm.org:8011/builders/clang-x86_64-debian-new-pass-manager-fast/builds/232/steps/test-check-all/logs/stdio

Fixed the indentation. Thanks.

Changed it. Thanks.

Thanks for the review. Please let me know if I have addressed everything.

After an (offline) review from @erichkeane (added now as a reviewer) , stripped the unnecessary code. Thanks.