This is an archive of the discontinued LLVM Phabricator instance.

Prevent ICE in dllexport class with _Atomic() data member
ClosedPublic

Authored by wristow on Jan 26 2017, 6:38 PM.

Details

Summary

Guard against a null pointer dereference that caused Clang to crash
when processing a class containing an _Atomic(<typ>) data member,
and that is tagged with 'dllexport'.

Diff Detail

Event Timeline

wristow created this revision.Jan 26 2017, 6:38 PM

When a class that has been tagged as dllexport (for an MSVC target) contains an atomic data member via the C11 '_Atomic' approach, the front end crashes with a null pointer dereference.
This patch fixes it by guarding the null dereference with the approach used by similar code in the same method.

rjmccall requested changes to this revision.Feb 1 2017, 11:15 AM
rjmccall added inline comments.
lib/CodeGen/CGClass.cpp
1135

I would prefer:

if (MemberExpr *ME2 = dyn_cast<MemberExpr>(RHS)) {
  if (ME2->getMemberDecl() == Field)
    return Field;
}
return nullptr;
This revision now requires changes to proceed.Feb 1 2017, 11:15 AM
wristow added inline comments.Feb 1 2017, 5:06 PM
lib/CodeGen/CGClass.cpp
1135

I see that change removes the dyn_cast<FieldDecl>. Was that intended, or an oversight?

In terms of changing the code-structure, in code on it's own, I do like the approach you described. But in this case, there is a sequence of if (<condition1>) return nullptr; ... if (conditionN) return nullptr; return Field;. Then after the block containing that set of guarded nullptr returns with a final return Field;, there is a similar block. And then there is a third block with a similar set. So changing the structure in that way breaks that pattern. With that in mind, do you still want that change done?

rjmccall added inline comments.Feb 1 2017, 5:13 PM
lib/CodeGen/CGClass.cpp
1135

The dyn_cast has no effect. There is no situation in which the declarations would compare equal without it where they would not with it, because Field is already known to be a FieldDecl.

The structure of the existing code is unlikely to stay the same. Actually, that code is quite worrying — it's making a lot of assumptions about how Sema synthesizes defaulted assignment operator bodies. But I didn't want to ask you to fix it when it's not the subject of your bug.

wristow updated this revision to Diff 86767.Feb 1 2017, 9:31 PM
wristow edited edge metadata.

Code restructured.

wristow marked 2 inline comments as done.Feb 1 2017, 9:33 PM
wristow added inline comments.
lib/CodeGen/CGClass.cpp
1135

Got it. Posted updated patch.

rjmccall accepted this revision.Feb 1 2017, 9:37 PM

Thanks, looks good.

This revision is now accepted and ready to land.Feb 1 2017, 9:37 PM
This revision was automatically updated to reflect the committed changes.
wristow marked an inline comment as done.