This is an archive of the discontinued LLVM Phabricator instance.

[clang] use getCommonSugar in an assortment of places
ClosedPublic

Authored by mizvekov on Oct 10 2021, 6:36 AM.

Details

Summary

For this patch, a simple search was performed for patterns where there are
two types (usually an LHS and an RHS) which are structurally the same, and there
is some result type which is resolved as either one of them (typically LHS for
consistency).

We change those cases to resolve as the common sugared type between those two,
utilizing the new infrastructure created for this purpose.

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

Diff Detail

Event Timeline

mizvekov created this revision.Oct 10 2021, 6:36 AM
mizvekov retitled this revision from [clang] WIP: Use getCommonSugar in an assortment of places to [clang] WIP: use getCommonSugar in an assortment of places.Oct 10 2021, 6:37 AM
mizvekov published this revision for review.Oct 12 2021, 4:00 PM
mizvekov retitled this revision from [clang] WIP: use getCommonSugar in an assortment of places to [clang] use getCommonSugar in an assortment of places.
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptOct 12 2021, 4:00 PM
Herald added subscribers: cfe-commits, Restricted Project. · View Herald Transcript
mizvekov edited the summary of this revision. (Show Details)Oct 31 2021, 9:03 AM
rsmith accepted this revision.Nov 12 2021, 2:03 PM
This revision is now accepted and ready to land.Nov 12 2021, 2:03 PM
mizvekov updated this revision to Diff 387579.Nov 16 2021, 5:01 AM
  • Run libcxx CI
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2021, 5:01 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
This revision now requires review to proceed.Nov 16 2021, 5:01 AM
shafik added a subscriber: shafik.Dec 2 2021, 2:12 PM

Changing how types are printed can effect LLDB expression parsing tests. I ran the LLDB test suite with this change:

ninja check-lldb

and it changes to the results for TestScalarURem.py which can be run using:

llvm-lit -sv lldb/test --filter TestScalarURem.py

The change looks expected.

mizvekov updated this revision to Diff 443769.Jul 11 2022, 3:10 PM
mizvekov edited the summary of this revision. (Show Details)
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 11 2022, 3:10 PM

llvm-lit -sv lldb/test --filter TestScalarURem.py

The change looks expected.

Thanks for letting me know. Unfortunately that test is not supported on my platform (Windows).
I have added a dummy file to lldb dir to try to trigger pre-commit CI there, and hopefully see what the error is.

mizvekov updated this revision to Diff 443772.Jul 11 2022, 3:29 PM
mizvekov updated this revision to Diff 444066.Jul 12 2022, 1:16 PM
rsmith accepted this revision.Jul 12 2022, 1:35 PM

Looks good to me.

clang/lib/Sema/SemaExprCXX.cpp
6560–6561

Can we remove this if now? getCommonSugaredType should unify the exception specifications so I think this stops being a special case.

6606–6607

Likewise here, can we remove this if?

clang/test/SemaObjC/format-strings-objc.m
271

Not related to this patch, but this new diagnostic is in some ways worse than the old one: casting to long doesn't fix the problem, given that the format specifier here %d expects an int.

mizvekov updated this revision to Diff 445365.Jul 17 2022, 5:51 PM
mizvekov marked 2 inline comments as done.Jul 17 2022, 5:51 PM
mizvekov updated this revision to Diff 445643.Jul 18 2022, 3:37 PM
mizvekov updated this revision to Diff 445648.Jul 18 2022, 4:04 PM
mizvekov updated this revision to Diff 447087.Jul 23 2022, 10:17 AM

Changing how types are printed can effect LLDB expression parsing tests. I ran the LLDB test suite with this change:

ninja check-lldb

and it changes to the results for TestScalarURem.py which can be run using:

llvm-lit -sv lldb/test --filter TestScalarURem.py

The change looks expected.

Fixed now, thanks. (And also API/commands/expression/rdar44436068/Test128BitsInteger.py).

mizvekov updated this revision to Diff 447974.Jul 27 2022, 2:56 AM
mizvekov updated this revision to Diff 453829.Aug 18 2022, 4:19 PM
mizvekov updated this revision to Diff 456197.Aug 28 2022, 10:27 AM
mizvekov updated this revision to Diff 458608.Sep 7 2022, 4:52 PM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 8 2022, 10:18 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
tra added a subscriber: tra.Sep 8 2022, 11:56 AM

It appears that this change may result in clang crashing (e.g. it broke CUDA compilation: https://lab.llvm.org/buildbot/#/builders/55/builds/35047/steps/3/logs/stdio).

Reproducer:

class {
  typedef __complex__ a;
  operator=(double b){c *= b} a c
$bin/clang++ -cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -std=c++14 -disable-llvm-passes -x c++ reproducer.cc
reproducer.cc:2:11: warning: plain '_Complex' requires a type specifier; assuming '_Complex double'
  typedef __complex__ a;
          ^
                      double
reproducer.cc:3:3: error: a type specifier is required for all declarations
  operator=(double b){c *= b} a c
  ^
reproducer.cc:3:34: error: expected ';' at end of declaration list
  operator=(double b){c *= b} a c
                                 ^
                                 ;
reproducer.cc:4:1: error: expected '}'
^
reproducer.cc:1:7: note: to match this '{'
class {
      ^
clang++: work/llvm/repo/llvm/include/llvm/Support/Casting.h:566: decltype(auto) llvm::cast(const From &) [To = clang::BuiltinType, From = clang::QualType]: Assertion `isa<To>(Val) && "cast<Ty>() argument of incompatible type!"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: bin/clang++ -cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -std=c++14 -disable-llvm-passes -x c++ reproducer.cc
1.      reproducer.cc:3:29: current parser token '}'
2.      reproducer.cc:1:1: parsing struct/union/class body '(anonymous)'
3.      reproducer.cc:3:22: parsing function body '(anonymous class)::operator='
4.      reproducer.cc:3:22: in compound statement ('{}')
 #0 0x000055ad5d7fe49a llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) work/llvm/repo/llvm/lib/Support/Unix/Signals.inc:569:11
 #1 0x000055ad5d7fe64b PrintStackTraceSignalHandler(void*) work/llvm/repo/llvm/lib/Support/Unix/Signals.inc:636:1
 #2 0x000055ad5d7fcc96 llvm::sys::RunSignalHandlers() work/llvm/repo/llvm/lib/Support/Signals.cpp:103:5
 #3 0x000055ad5d7fed75 SignalHandler(int) work/llvm/repo/llvm/lib/Support/Unix/Signals.inc:407:1
 #4 0x00007f4b4e083200 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12200)
 #5 0x00007f4b4daf28c1 raise ./signal/../sysdeps/unix/sysv/linux/raise.c:50:1
 #6 0x00007f4b4dadc546 abort ./stdlib/abort.c:81:7
 #7 0x00007f4b4dadc42f get_sysdep_segment_value ./intl/loadmsgcat.c:509:8
 #8 0x00007f4b4dadc42f _nl_load_domain ./intl/loadmsgcat.c:970:34
 #9 0x00007f4b4daeb242 (/lib/x86_64-linux-gnu/libc.so.6+0x31242)
#10 0x000055ad5e2f39e8 decltype(auto) llvm::cast<clang::BuiltinType, clang::QualType>(clang::QualType const&) work/llvm/repo/llvm/include/llvm/Support/Casting.h:567:43
#11 0x000055ad5e2f3929 clang::BuiltinType const* clang::Type::castAs<clang::BuiltinType>() const work/llvm/build/debug/tools/clang/include/clang/AST/TypeNodes.inc:86:1
#12 0x000055ad62cfe279 clang::ASTContext::getFloatTypeSemantics(clang::QualType) const work/llvm/repo/clang/lib/AST/ASTContext.cpp:1707:14
#13 0x000055ad61e8ad61 unsupportedTypeConversion(clang::Sema const&, clang::QualType, clang::QualType) work/llvm/repo/clang/lib/Sema/SemaExpr.cpp:1256:29
#14 0x000055ad61eb3961 clang::Sema::CheckAssignmentConstraints(clang::QualType, clang::ActionResult<clang::Expr*, true>&, clang::CastKind&, bool) work/llvm/repo/clang/lib/Sema/SemaExpr.cpp:9619:7
#15 0x000055ad61eb30c5 clang::Sema::CheckAssignmentConstraints(clang::SourceLocation, clang::QualType, clang::QualType) work/llvm/repo/clang/lib/Sema/SemaExpr.cpp:9465:3
#16 0x000055ad61ec565f clang::Sema::CheckAssignmentOperands(clang::Expr*, clang::ActionResult<clang::Expr*, true>&, clang::SourceLocation, clang::QualType, clang::BinaryOperatorKind) work/llvm/repo/clang/lib/Sema/SemaExpr.cpp:13919:12
#17 0x000055ad61ea1903 clang::Sema::CreateBuiltinBinOp(clang::SourceLocation, clang::BinaryOperatorKind, clang::Expr*, clang::Expr*) work/llvm/repo/clang/lib/Sema/SemaExpr.cpp:14988:11
#18 0x000055ad61ecaf02 clang::Sema::BuildBinOp(clang::Scope*, clang::SourceLocation, clang::BinaryOperatorKind, clang::Expr*, clang::Expr*) work/llvm/repo/clang/lib/Sema/SemaExpr.cpp:15511:10
#19 0x000055ad61e8a180 clang::Sema::ActOnBinOp(clang::Scope*, clang::SourceLocation, clang::tok::TokenKind, clang::Expr*, clang::Expr*) work/llvm/repo/clang/lib/Sema/SemaExpr.cpp:15323:10

Thanks for the repro!

ronlieb added a subscriber: ronlieb.Sep 8 2022, 1:33 PM

i am seeing similar issues in openmp buildbot failing with this patch

i am seeing similar issues in openmp buildbot failing with this patch

I got a small patch coming up soon.

MaskRay added a subscriber: MaskRay.Sep 8 2022, 5:07 PM

Since the fix was not merged, I will revert this soon.

I saw a clang segfault with clang++ libstdc++-v3/src/c++98/complex_io.cc

Since the fix was not merged, I will revert this soon.

I saw a clang segfault with clang++ libstdc++-v3/src/c++98/complex_io.cc

Would you mind if I just merge it? I was hoping there would be someone around to have a quick look, but apparently not.

Since the fix was not merged, I will revert this soon.

I saw a clang segfault with clang++ libstdc++-v3/src/c++98/complex_io.cc

Would you mind if I just merge it? I was hoping there would be someone around to have a quick look, but apparently not.

Sorry, I just reverted it:) You may check whether the fix fixes GCC libstdc++-v3/src/c++98/complex_io.cc

mizvekov reopened this revision.Sep 8 2022, 5:15 PM
mizvekov updated this revision to Diff 458924.Sep 8 2022, 5:19 PM
mizvekov updated this revision to Diff 459070.Sep 9 2022, 8:11 AM
mizvekov edited the summary of this revision. (Show Details)
mizvekov added inline comments.Sep 9 2022, 8:21 AM
clang/lib/Sema/SemaExpr.cpp
1091–1160

@shafik let's continue revision of D133522 in this patch, since that one was a quick fix that I attempted in order to avoid reverting it. But now that it was reverted, I will abandon the other one.

I have highlighted with this comment the areas affected by D133522 so we can continue here.

But now I have refactored the whole thing to be simpler / more readable.

Sorry, I just reverted it:) You may check whether the fix fixes GCC libstdc++-v3/src/c++98/complex_io.cc

I confirm it does not crash on my system with this patch.

mizvekov updated this revision to Diff 459362.Sep 11 2022, 7:46 AM
mizvekov updated this revision to Diff 459369.Sep 11 2022, 7:53 AM
mizvekov marked an inline comment as not done.Sep 13 2022, 6:42 AM
mizvekov added a subscriber: davrec.
mizvekov added inline comments.
clang/lib/Sema/SemaExpr.cpp
1091–1160

Also @davrec if you can please take a look at this, this is the only part of the patch that changed, in order to fix the reason why it was reverted.

The lines you changed (clang/lib/Sema/SemaExpr.cpp:1091-1150) look good (IIUC you just change cast to cast_as and dyn_cast to getAs, and reorganize for clarity). I suggested some renaming and a few more comments to further clarify.

clang/lib/Sema/SemaExpr.cpp
1113–1114

Rename params for clarity, e.g.
LHS->Longer
LHSType->LongerType
RHSType->ShorterType

1116–1131
// Compute the rank of the two types, regardless of whether they are complex.
1159
// Promote the precision of the LHS if not an assignment.
1161
// Promote the precision of the RHS unless it is already the same as the LHS.
davrec added inline comments.Sep 13 2022, 8:02 AM
clang/lib/Sema/SemaExpr.cpp
1113–1114

Actually I probably have that backwards, I think LHS is the Shorter expression...in any case you see why renaming would be helpful

mizvekov updated this revision to Diff 460036.Sep 14 2022, 4:39 AM
mizvekov marked an inline comment as not done.
mizvekov marked 5 inline comments as done.Sep 14 2022, 4:40 AM
mizvekov updated this revision to Diff 460220.Sep 14 2022, 2:04 PM
mizvekov updated this revision to Diff 460587.Sep 15 2022, 6:10 PM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 16 2022, 2:56 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
mizvekov reopened this revision.Sep 16 2022, 3:59 AM
mizvekov updated this revision to Diff 460723.Sep 16 2022, 6:25 AM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 16 2022, 7:37 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.