This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Fix crash when checking misaligned member with dependent type
ClosedPublic

Authored by junaire on Oct 15 2022, 6:33 AM.

Details

Summary

If the type is dependent, we should just discard it and not checking its
alignment as it doesn't exisit yet.
Fixes https://github.com/llvm/llvm-project/issues/58370

Diff Detail

Event Timeline

junaire created this revision.Oct 15 2022, 6:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2022, 6:33 AM
junaire requested review of this revision.Oct 15 2022, 6:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2022, 6:33 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added inline comments.Oct 17 2022, 5:56 AM
clang/lib/Sema/SemaChecking.cpp
17397

We don't typically use top-level const qualification.

17398–17399

Hmm, is this logic correct? Don't we want:

T->isDependentType() ||
  (T->isPointerType() &&
    (T->getPointeeType()->isIncompleteType() || Context.getTypeAlignInChars(T->getPointeeType()) <= MA->Alignment))
junaire updated this revision to Diff 468193.Oct 17 2022, 7:15 AM

Address Aaron's comments, thanks!

shafik added inline comments.Oct 17 2022, 11:39 AM
clang/lib/Sema/SemaChecking.cpp
17398–17399

Why did you drop the T->isIntegerType()?

clang/test/SemaCXX/misaligned-member-with-depdent-type.cpp
4

I think since the bug report had malformed code, it would be worth it to include that case as well.

aaron.ballman added inline comments.Oct 17 2022, 11:41 AM
clang/lib/Sema/SemaChecking.cpp
17398–17399

Whoops! That's totally a typo on my part, I didn't intend to drop it.

junaire updated this revision to Diff 468409.Oct 17 2022, 9:29 PM

Add the original mailformed test case, thanks Shafik!

junaire marked 5 inline comments as done.Oct 17 2022, 9:35 PM
junaire added inline comments.
clang/lib/Sema/SemaChecking.cpp
17398–17399

Why did you drop the T->isIntegerType()?

There may be some misunderstanding here. So previously I hoist the pointer-related checks and put T->isDependentType() into it. After Aaron's words, I realized it was wrong so I changed the whole check condition to is T an integer type? || is T a dependent type ?|| is T a pointer type and other constraints (put them on the same level)

That said you're looking at the old diff so there's nothing need to worried about :)

This revision is now accepted and ready to land.Oct 18 2022, 4:32 AM
This revision was landed with ongoing or failed builds.Oct 18 2022, 6:07 AM
This revision was automatically updated to reflect the committed changes.
junaire marked an inline comment as done.