Page MenuHomePhabricator

[clang/DIVar] Emit flag for params that have unchanged values
Needs ReviewPublic

Authored by djtodoro on Mon, Feb 11, 3:21 AM.

Details

Summary

Emit debug info flag that indicates that a parameter has unchanged value through the course of a function.

This info will be used for emitting DW_OP_entry_value expressions for the parameter.

Authors: @asowda, @NikolaPrica, @djtodoro, @ivanbaev

Diff Detail

Event Timeline

djtodoro created this revision.Mon, Feb 11, 3:21 AM
djtodoro added a project: debug-info.
djtodoro added a subscriber: petarj.
djtodoro updated this revision to Diff 186611.Wed, Feb 13, 2:42 AM
  • Rename: VariableNotChanged ===> ArgumentNotModified
  • Refactor a test case
probinson added inline comments.Thu, Feb 21, 10:36 AM
include/clang/AST/Decl.h
1483

There should be a comment here.
The style in this class appears to omit the 'get' word from the name of the getter, so isArgumentModified for the method name would look more consistent.

lib/Sema/SemaExpr.cpp
11282

Does this fit on one line if you use const auto *LHSDeclRef ...? Given you have the dyn_cast on the RHS there's no real need to give the type name twice.

+ rsmith, + bricci for review.

I was under the impression that space inside VarDecl was quite constrained. Pardon the likely naive question, but: is there any way to make the representation more compact (maybe sneak a bit into ParmVarDeclBitfields)?

riccibruno edited reviewers, added: riccibruno; removed: bricci.Thu, Feb 21, 1:04 PM

If this bit is relevant to function parameters, why is getIsArgumentModified in VarDecl and not in ParamVarDecl ? What you can do is move the relevant methods to ParamVarDecl, and stash the bit in ParmVarDeclBitfields.

include/clang/AST/Decl.h
843

Yeah, please don't waste a whole pointer just to store a bit.

Oh and I think that you will also have to update the serialization code/de-serialization code in ASTReaderDecl.cpp / ASTWriterDecl.cpp. You might also have to update TreeTransform but I am less familiar with this.

I am wondering about the semantics of this bit. I don't think that you can know for sure within clang whether a variable has been modified. The best you can do is know for sure that some variable has been modified, but I don't think you can prove that it has not been modified.

Consequently I am wondering if you should change the name of this flag to something like IsArgumentKnownToBeModified.

Orthogonal to this you need to also add tests for templates.

djtodoro marked 2 inline comments as done.Fri, Feb 22, 6:22 AM

I was under the impression that space inside VarDecl was quite constrained. Pardon the likely naive question, but: is there any way to make the representation more compact (maybe sneak a bit into ParmVarDeclBitfields)?

@vsk Thanks for the comment! Sure, it is better idea. Initially, we thought this could be used even for local variables, but since we are using it only for parameters it makes more sense.

lib/Sema/SemaExpr.cpp
11282

Does this fit on one line if you use const auto *LHSDeclRef ...? Given you have the dyn_cast on the RHS there's no real need to give the type name twice.

@probinson Thanks for the comment! Sure, it will be refactored.

djtodoro marked an inline comment as done.Fri, Feb 22, 6:27 AM

@riccibruno Thanks for your comments!

Oh and I think that you will also have to update the serialization code/de-serialization code in ASTReaderDecl.cpp / ASTWriterDecl.cpp. You might also have to update TreeTransform but I am less familiar with this.

I've added ASTReaderDecl.cpp / ASTWriterDecl.cpp, but looking at TreeTransform I'm not sure if there is a place for adding something regarding this.

I am wondering about the semantics of this bit. I don't think that you can know for sure within clang whether a variable has been modified. The best you can do is know for sure that some variable has been modified, but I don't think you can prove that it has not been modified.

Consequently I am wondering if you should change the name of this flag to something like IsArgumentKnownToBeModified.

Orthogonal to this you need to also add tests for templates.

I agree! You are right, good idea!

djtodoro updated this revision to Diff 187931.Fri, Feb 22, 6:31 AM
  • Add a field in ParmVarDecl instead of VarDecl
  • Use a bit in ParmVarDeclBitfields to indicate parameter modification
  • Add support for the bit in ASTReaderDecl.cpp / ASTWriterDecl.cpp
  • Add test case for templates
riccibruno added inline comments.Fri, Feb 22, 6:51 AM
lib/Sema/SemaExpr.cpp
11301

Comments:

  1. Shouldn't you mark the variable to be modified only if CheckForModifiableLvalue returns true ?
  1. I think that you need to handle way more than just member expressions. For example are you handling (x, y) (comma operator) ? But hard-coding every cases here is probably not ideal. It would be nice if there was already some code somewhere that could help you do this.
  1. I believe that a MemberExpr has always a base. Similarly DeclRefExprs always have a referenced declaration (so you can omit the if).
lebedev.ri added inline comments.
lib/Sema/SemaExpr.cpp
11301

I'm not quite sure what this differential is about, but i feel like mentioning ExprMutationAnalyzer lib in clang-tidy / clang-tools-extra.