This is an archive of the discontinued LLVM Phabricator instance.

[clang] Changes to produce sugared converted template arguments
ClosedPublic

Authored by mizvekov on Sep 14 2022, 9:43 AM.

Details

Summary

Makes CheckTemplateArgumentList and the template deduction functions
produce a sugared converted argument list in addition to the canonical one.

This is mostly NFC except that we hook this up to a few diagnostics in
SemaOverload.

The infrastructure here will be used in subsequent patches
where we perform a finalized sugared substitution for entities
which we do not unique per specializations on canonical arguments,
and later on will be used for template specialization resugaring.

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Diff Detail

Event Timeline

mizvekov created this revision.Sep 14 2022, 9:43 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptSep 14 2022, 9:43 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mizvekov requested review of this revision.Sep 14 2022, 9:43 AM
mizvekov updated this revision to Diff 460218.Sep 14 2022, 2:03 PM
mizvekov retitled this revision from WIP: [clang] Implement sugar retention for converted template arguments to [clang] Implement sugar retention for converted template arguments.
mizvekov updated this revision to Diff 460594.Sep 15 2022, 6:10 PM
mizvekov updated this revision to Diff 460911.Sep 16 2022, 2:34 PM
mizvekov updated this revision to Diff 461968.Sep 21 2022, 11:44 AM
mizvekov updated this revision to Diff 466364.Oct 9 2022, 5:44 AM
mizvekov removed a reviewer: Restricted Project.Oct 9 2022, 5:45 AM
mizvekov updated this revision to Diff 467919.Oct 14 2022, 2:31 PM
mizvekov updated this revision to Diff 470006.Oct 23 2022, 1:21 PM
mizvekov retitled this revision from [clang] Implement sugar retention for converted template arguments to [clang] Changes to produce sugared converted template arguments.
mizvekov edited the summary of this revision. (Show Details)
mizvekov added reviewers: davrec, Restricted Project.Oct 23 2022, 1:23 PM
mizvekov updated this revision to Diff 470346.Oct 24 2022, 6:34 PM
mizvekov added a subscriber: erichkeane.

@erichkeane since you reviewed and approved 5 patches that depend on this one, can you also have a look at this please?

Thanks!

@erichkeane since you reviewed and approved 5 patches that depend on this one, can you also have a look at this please?

Thanks!

Thanks for the ping, I missed this is in the massive influx of emails I got! I'll take a look when i get a chance.

The test changes are a little bizarre, are there any better tests you can write that shows this behavior?

Also, at the 'end' of this patch set, we should make sure we have a detailed release note.

clang/include/clang/Sema/Sema.h
8281

I find myself wondering if we should just be passing around the 'Sugared' version, and canonicalizing when we need it? Is there a good reason not to?

clang/include/clang/Sema/TemplateDeduction.h
94–102

Not your fault at all.... but that seems to be about the absolute opposite of what it looks like this function is doing...

clang/lib/Sema/SemaTemplate.cpp
4876

At one point I wonder if there is value to storing the sugared version here instead. We currently just create these every time we need them, so the side-effect would be possibly a nicer looking AST dump. BUT we don't really canonizalize these Specialization Decls/Specialization Exprs.

mizvekov added inline comments.Oct 25 2022, 8:41 AM
clang/include/clang/Sema/Sema.h
8281

I think given the size of the patch, splitting things this way makes it way easier to not pass the wrong list somewhere, and then spend a long time looking for the problem.

I plan to make this better in a future patch, I am investigating the net benefits of tracking and uniquing template argument lists, much like how we do for types.

Then, I think we can get rid of this duplication in all places that are just passing the lists around.

Otherwise, llvm-compile-time-tracker shows no performance impact from this patch at all (and the same for all the others in the sugared substitution series), so I did not want to block everything on that extra work.

clang/include/clang/Sema/TemplateDeduction.h
94–102

Hah, it's written in the perspective of the user, not the provider.

The opposite of standard practice I guess.

clang/lib/Sema/SemaTemplate.cpp
4876

That will happen in D136566, which you already reviewed.

If we did that before the changes in D134604, then this would lead to a crash for certain test cases involving partial substitutions.

erichkeane accepted this revision.Oct 25 2022, 8:42 AM
erichkeane added inline comments.
clang/include/clang/Sema/Sema.h
8281

SGTM.

clang/lib/Sema/SemaTemplate.cpp
4876

Ah! Hoisted by my own poor memory again! Thanks.

This revision is now accepted and ready to land.Oct 25 2022, 8:42 AM

The test changes are a little bizarre, are there any better tests you can write that shows this behavior?

Also, at the 'end' of this patch set, we should make sure we have a detailed release note.

Well, this is mostly just a pure enabler, the rest of the patch series implement the uses for the Sugared lists.

It's just that there were 3 places in SemaOverload where we are just diagnosing a deduction failure, and we could have just used the sugared lists there instead.
Otherwise, these changes would not fit too well with any of the other patches, and I thought it would have been a bit unnecessary to split that tiny patch off.

But I guess it's fair to say that the incidental testing is not covering those new uses very well, I will add some extra changes later.

mizvekov updated this revision to Diff 470645.Oct 25 2022, 4:42 PM

This broke building Firefox with:

In file included from Unified_cpp_editor_txmgr0.cpp:2:
In file included from /tmp/gecko/editor/txmgr/TransactionItem.cpp:6:
In file included from /tmp/gecko/editor/txmgr/TransactionItem.h:9:
In file included from /tmp/gecko/obj-x86_64-pc-linux-gnu/dist/include/nsCOMPtr.h:31:
In file included from /tmp/gecko/obj-x86_64-pc-linux-gnu/dist/include/nsISupportsUtils.h:16:
/tmp/gecko/obj-x86_64-pc-linux-gnu/dist/include/nsISupportsImpl.h:238:31: error: definition with same mangled name '_ZN27nsCycleCollectingAutoRefCnt4incrIXadL_Z25NS_CycleCollectorSuspect3EEEEmPvP28nsCycleCollectionParticipant' as another definition
  MOZ_ALWAYS_INLINE uintptr_t incr(void* aOwner,
                              ^
/tmp/gecko/obj-x86_64-pc-linux-gnu/dist/include/nsISupportsImpl.h:238:31: note: previous definition is here
/tmp/gecko/obj-x86_64-pc-linux-gnu/dist/include/nsISupportsImpl.h:266:31: error: definition with same mangled name '_ZN27nsCycleCollectingAutoRefCnt4decrIXadL_Z25NS_CycleCollectorSuspect3EEEEmPvP28nsCycleCollectionParticipantPb' as another definition
  MOZ_ALWAYS_INLINE uintptr_t decr(void* aOwner,
                              ^
/tmp/gecko/obj-x86_64-pc-linux-gnu/dist/include/nsISupportsImpl.h:266:31: note: previous definition is here
2 errors generated.

This broke building Firefox with:

Thanks for reporting. Do you have preprocessed source + cc1 flags to reproduce it, so I can have a look?

I'm running it through creduce (as well as the one for D136564. It's going to take a little while. Thank you for the revert.

I'm running it through creduce (as well as the one for D136564. It's going to take a little while. Thank you for the revert.

That's very helpful, thank you!

Reduced testcase:
cc1 command line: clang-16 -cc1 -triple x86_64-unknown-linux-gnu -emit-obj -o Unified_cpp_editor_txmgr0.o -x c++-cpp-output Unified_cpp_editor_txmgr0.ii
source file content:

class nsCycleCollectionParticipant;
class nsCycleCollectingAutoRefCnt;
extern "C" void NS_CycleCollectorSuspect3(void *,
                                          nsCycleCollectionParticipant *,
                                          nsCycleCollectingAutoRefCnt *,
                                          bool *);
class nsCycleCollectingAutoRefCnt {
public:
  typedef void Suspect(void *, nsCycleCollectionParticipant *,
                       nsCycleCollectingAutoRefCnt *, bool *);
  template <Suspect suspect = NS_CycleCollectorSuspect3>
  void incr(int *aOwner) {
    incr<suspect>(aOwner, nullptr);
  }
  template <Suspect = NS_CycleCollectorSuspect3>
  unsigned long incr(void *, nsCycleCollectionParticipant *) {}
};
class TransactionItem {
  int Release();
  nsCycleCollectingAutoRefCnt mRefCnt;
};
int TransactionItem::Release() {
  mRefCnt.incr(this, 0);
  mRefCnt.incr(0);
}

Thanks.

Reduced to:

void foo();
template <void () = foo> void bar() {}
template void bar<>();
template void bar<foo>();

This is surprisingly not from a bug in this patch.

So getCanonicalTemplateArgument is broken for the types of declarations.

This bug was supposedly fixed by zygoloid (https://github.com/llvm/llvm-project/commit/849c60541b630ddf8cabf9179fa771b3f4207)
But then was reverted by https://github.com/llvm/llvm-project/commit/ba2dff0159fcd1d2349bc610331914618ca9bc30.

I think there is a high chance that the bug that caused richard's patch to be reverted, was actually fixed afterwards by https://github.com/llvm/llvm-project/commit/acb767f5cda59302aa9100afcf6133d66779d42c

So I will try to land that one again, and then I will try to land this one again if all goes well.

glandium added a comment.EditedOct 26 2022, 5:38 PM

I can confirm that applying the following (derived from 849c60541b630ddf8cabf9179fa771b3f4207):

diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index a8585a6d84ad..f07c40cb6c5d 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -6774,7 +6774,7 @@ ASTContext::getCanonicalTemplateArgument(const TemplateArgument &Arg) const {
 
     case TemplateArgument::Declaration: {
       auto *D = cast<ValueDecl>(Arg.getAsDecl()->getCanonicalDecl());
-      return TemplateArgument(D, Arg.getParamTypeForDecl());
+      return TemplateArgument(D, getCanonicalType(Arg.getParamTypeForDecl()));
     }
 
     case TemplateArgument::NullPtr:
diff --git a/clang/lib/AST/TemplateBase.cpp b/clang/lib/AST/TemplateBase.cpp
index 6d9ab2b8ca71..579684c94f80 100644
--- a/clang/lib/AST/TemplateBase.cpp
+++ b/clang/lib/AST/TemplateBase.cpp
@@ -363,7 +363,8 @@ bool TemplateArgument::structurallyEquals(const TemplateArgument &Other) const {
            TemplateArg.NumExpansions == Other.TemplateArg.NumExpansions;
 
   case Declaration:
-    return getAsDecl() == Other.getAsDecl();
+    return getAsDecl() == Other.getAsDecl() &&
+           getParamTypeForDecl() == Other.getParamTypeForDecl();
 
   case Integral:
     return getIntegralType() == Other.getIntegralType() &&

on top of the commit right before your reverts, fixes both the issue I reported here as well as the one in D136564.

Thanks for confirming.

I have pushed a patch at: https://reviews.llvm.org/D136803