This is an archive of the discontinued LLVM Phabricator instance.

[clang] Do not crash when ArgTy is null in CheckArgAlignment
ClosedPublic

Authored by adamcz on Jun 7 2021, 9:27 AM.

Details

Summary

No repro case just yet. I only observed a crash like this and noticed
ArgTy-Value = 0 in coredump.

Diff Detail

Event Timeline

adamcz requested review of this revision.Jun 7 2021, 9:27 AM
adamcz created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2021, 9:27 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
kadircet added inline comments.
clang/lib/Sema/SemaChecking.cpp
4567

i think ArgTy becomes null after this operation. e.g. the function requires a pointer param, user provides a non-pointer arg in a context where recovery exprs would keep the ast call attached.

I've also spent quite some time trying to reproduce but no luck.

4657

i think before diving in here we should ensure this Expr doesn't have type or error dependence.

adamcz updated this revision to Diff 350612.Jun 8 2021, 8:36 AM

added a test

adamcz updated this revision to Diff 350613.Jun 8 2021, 8:38 AM

update commit description

adamcz updated this revision to Diff 350629.Jun 8 2021, 9:35 AM

Added some containsErrors() calls, not sure if that's the right thing to do

FYI The ArgTy.isNull() check is sufficient to fix this. The Arg->containsErrors() is not - it's false in this case, since it seems CXXDefaultArgExpr with RecoveryExpr inside seems to not contains errors, according to containsError(). I can't tell if that's by design, or a bug.

Haojian, please let me know if you think Arg->containsError() should be true or if we should be testing somewhere else, or just drop it and accept that CheckArgAlignment may be called with RecoveryExpr and have it check for ArgTy->isNull().

Basically, let me know what you think about this. Thanks!

kadircet accepted this revision.Jun 8 2021, 9:56 AM

as discussed offline LG from my side.

regarding the default arg of a parameter containing an error, it seems to be effecting type information of the parameter even though the type is explicitly specified. so it feels like either:

  • one cannot perform any semantic checks on a parmvardecl with an errornous default arg, hence error dependence bit should be propagated from default arg to the parameter, or
  • there's an issue with the type checking logic itself and it shouldn't be making type of the var decl depend on the default argument (when it is explicitly specified in the declaration).

but i am not an expert in either, so these are just feelings :). But it'd be great if we got rid of the additional default arg containing error check in here one way or the other.

This revision is now accepted and ready to land.Jun 8 2021, 9:56 AM

FYI The ArgTy.isNull() check is sufficient to fix this. The Arg->containsErrors() is not - it's false in this case, since it seems CXXDefaultArgExpr with RecoveryExpr inside seems to not contains errors, according to containsError(). I can't tell if that's by design, or a bug.

Haojian, please let me know if you think Arg->containsError() should be true or if we should be testing somewhere else, or just drop it and accept that CheckArgAlignment may be called with RecoveryExpr and have it check for ArgTy->isNull().

Basically, let me know what you think about this. Thanks!

Thanks for the analysis.

Yeah, I think this is a bug in clang when computing the dependence of CXXDefaultArgExpr -- the CXXDefaultArgExpr should respect the dependence of its actual argument (getExpr()). I think we should fix it first.

Regarding to test it, the best way is to dump the AST node and verify the containsError is in the dump string.

Thanks Haojian!
I sent out https://reviews.llvm.org/D103982 for the CXXDefaultArgExpr. I'll update this change after I submit that one.

adamcz updated this revision to Diff 351155.Jun 10 2021, 6:28 AM

updated after the CXXDefaultArgExpr containsErrors() change.

hokein accepted this revision.Jun 10 2021, 7:04 AM

Thanks! Really hope this is the last time to fix this crash.

clang/lib/Sema/SemaChecking.cpp
4574–4576

looks like the isNull is not needed to fix the crash, but let's be more defensive this time.