Page MenuHomePhabricator

Fix for Bug25668. Clang is asserting when lookup argument is a class that hasn't been instantiated.
AcceptedPublic

Authored by zahiraam on Nov 6 2020, 7:00 AM.

Details

Summary

Argument dependent lookup with class argument is visiting base classes that haven't been instantiated. This is generating an assertion in DeclTemplate.h.

Diff Detail

Event Timeline

zahiraam requested review of this revision.Nov 6 2020, 7:00 AM
zahiraam created this revision.
zahiraam updated this revision to Diff 303498.Nov 6 2020, 10:29 AM
rnk added a comment.Nov 6 2020, 10:45 AM

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.

clang/lib/Sema/SemaLookup.cpp
2730

I think a more straightforward way of testing if a SourceLocation is present would be to call InstantiationLoc.isValid(). Converting to a pointer and comparing against null is a bit more roundabout.

zahiraam updated this revision to Diff 303514.Nov 6 2020, 11:53 AM
zahiraam marked an inline comment as done.
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.

Yes I agree. I am working on it. I will open another bug for that.

FWIW, as with the code, the test should probably live in /OpenMP instead.

zahiraam updated this revision to Diff 304636.Nov 11 2020, 1:27 PM

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.

zahiraam updated this revision to Diff 307693.Nov 25 2020, 12:40 PM

I am not really convinced that this is an openmp issue.

As long as you need pragma omp to trigger this, it looks OpenMP specific. If it is a problem without, we should not use a pragma in the test case.

The best I can tell the underlying OpenMP problem is the lookup code in buildUserDefinedMapperRef. In this code if there is no mapper-id (like this test) the name is changed to "default" but the Loc is not set. If we set the location to something (StartLoc is handy here) all is well.

--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -18774,6 +18774,7 @@ static void checkMappableExpressionList(
     auto &DeclNames = SemaRef.getASTContext().DeclarationNames;
     MapperId.setName(DeclNames.getIdentifier(
         &SemaRef.getASTContext().Idents.get("default")));
+    MapperId.setLoc(StartLoc);
   }

   // Iterators to find the current unresolved mapper expression.
zahiraam updated this revision to Diff 308362.Nov 30 2020, 7:01 AM
This revision is now accepted and ready to land.Dec 1 2020, 10:28 AM