This is an archive of the discontinued LLVM Phabricator instance.

[NFC][Clang] Fix static analyzer concerns
ClosedPublic

Authored by eandrews on Aug 4 2023, 9:47 AM.

Details

Summary

Fix static analyzer concerns about dereferencing null values

Diff Detail

Event Timeline

eandrews created this revision.Aug 4 2023, 9:47 AM
Herald added a project: Restricted Project. · View Herald Transcript
eandrews requested review of this revision.Aug 4 2023, 9:47 AM
aaron.ballman accepted this revision.Aug 4 2023, 11:39 AM

LGTM though there may be some additional changes worth considering; I don't insist on changes though.

clang/lib/AST/StmtPrinter.cpp
178

Hmmm, I think this is what we're effectively already hoping is the case today, but I don't think that's a safe assertion by itself. Consider: https://github.com/llvm/llvm-project/blob/cd29ebb862b5c7a81c9f39c8c493f9246d6e5f0b/clang/lib/AST/StmtPrinter.cpp#L602

It might be worth looking at all the calls to PrintRawCompoundStmt() to see which ones potentially can pass null in, and decide if there are additional changes to make. e.g., should that dyn_cast be a cast instead so it cannot return nullptr and we get the assertion a little bit earlier when calling cast<>?

This revision is now accepted and ready to land.Aug 4 2023, 11:39 AM
tahonermann accepted this revision.Aug 8 2023, 9:10 AM

I think this change is good as is, but I added some additional thoughts to Aaron's earlier comment.

clang/lib/AST/StmtPrinter.cpp
178

Another possibility would be to modify ObjCAtFinallyStmt to declare AtFinallyStmt as CompoundStmt* and to modify getFinallyBody() and setFinallyBody() accordingly. That would remove the need for that dyn_cast in StmtPrinter::VisitObjCAtTryStmt(). This would then require additional changes such as updates to ASTStmtReader::VisitObjCAtFinallyStmt() to perform a cast as is done in ASTStmtReader::VisitStmtExpr(). But none of that actually addresses the fact that this function has a precondition that Node is not null.

eandrews updated this revision to Diff 554063.Aug 28 2023, 2:59 PM

Changed a dyn_cast to cast and handled ObjCAtFinallyStmt similar to how ObjCAtTryStmt is handled, i.e. just check that it exists before calling PrintRawCompoundStmt

This revision was landed with ongoing or failed builds.Aug 29 2023, 12:23 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2023, 12:23 PM