C++: Two IR fixes and a PrintAST workaround#1674
Conversation
My original fix in github#1661 fixed my minimal test case, but did not fix the original failure in a Linux snapshot. The real fix is to simply not create a `TranslatedDeclarationEntry` for an extern declaration, and have `TranslatedDeclStmt` skip any such declarations. I've added a regression test for that case (multiple extern declarations with same location in a macro expansion, with control flow between them). I did verify that it generates correct IR, and that it fixes all of the "use not dominated by definition" failures in Linux. The underlying extractor bug, that caused the above issue also caused PrintAST to print garbage. I've worked around the bug in PrintAST.qll. I've also fixed a bug in the control flow for `try`/`catch`, where there was missing flow from the `CatchByType` of the last handler of a `try` to the enclosing handler (or `Unwind`). Hat tip to @andreidiaconu1 for spotting this bug.
dave-bartolomeo
changed the title
|
@zlaski-semmle do the PrintAST changes look OK? |
|
Alas, I'm insufficiently knowledgeable to review this. |
|
Another question is whether the changes to |
|
The extractor issue is unlikely to be fixed soon. My understanding is that it's not easy to efficiently compute the correct parent for this specific case given EDG's current AST representation. Until it's fixed, any PrintAST invocation that hits this code pattern produces nodes scattered everywhere, disconnected from parents, with multiple copies. This is because we produce a depth-first numbering of the complete tree, and when the tree is not really a tree but a DAG, our numbering algorithm produces multiple numbers for a single node. It was completely unusable when I tried it. |
|
Anybody in @Semmle/cpp available to review the PrintAST changes here? |
| DeclarationEntry entry; | ||
| class DeclarationEntryNode extends BaseASTNode, TDeclarationEntryNode { | ||
| override DeclarationEntry ast; | ||
| DeclStmt declStmt; |
There was a problem hiding this comment.
Is the declStmt field used? I can't find a use.
3 participants
My original fix in #1661 fixed my minimal test case, but did not fix the original failure in a Linux snapshot. The real fix is to simply not create a
TranslatedDeclarationEntryfor an extern declaration, and haveTranslatedDeclStmtskip any such declarations. I've added a regression test for that case (multiple extern declarations with same location in a macro expansion, with control flow between them). I did verify that it generates correct IR, and that it fixes all of the "use not dominated by definition" failures in Linux.The underlying extractor bug, that caused the above issue also caused PrintAST to print garbage. I've worked around the bug in PrintAST.qll.
I've also fixed a bug in the control flow for
try/catch, where there was missing flow from theCatchByTypeof the last handler of atryto the enclosing handler (orUnwind). Hat tip to @andreidiaconu1 for spotting this bug.