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

[djtodoro]

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

[djtodoro]

• Rename: VariableNotChanged ===> ArgumentNotModified
• Refactor a test case

[probinson]

include/clang/AST/Decl.h

1482

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.

[vsk]

+ 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]

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.

[riccibruno]

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.

[riccibruno]

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]

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]

@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]

• 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]

lib/Sema/SemaExpr.cpp

11305

Comments:

1. Shouldn't you mark the variable to be modified only if CheckForModifiableLvalue returns true ?

2. 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.

3. I believe that a MemberExpr has always a base. Similarly DeclRefExprs always have a referenced declaration (so you can omit the if).

[lebedev.ri]

lib/Sema/SemaExpr.cpp

11305

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

[riccibruno]

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

Alternatively perhaps you could re-use getMemoryLocation() from D57660. It would handle for you members, references, structured bindings +more in a relatively self-contained code.

[djtodoro]

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

Alternatively perhaps you could re-use getMemoryLocation() from D57660. It would handle for you members, references, structured bindings +more in a relatively self-contained code.

@lebedev.ri, @riccibruno Thanks for your advices! We'll check this also.

lib/Sema/SemaExpr.cpp

11305

@riccibruno Please find inlined replies:

1. Shouldn't you mark the variable to be modified only if CheckForModifiableLvalue returns true ?

Hmm, we should avoid marking variables modification if this emits an error. But, we should emit it if CheckForModifiableLvalue returns false, since in the case of returning true an error will be emitted.

2. 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.

Hard coding all cases is not good idea. I agree. Since we are only looking for declaration references, we could make just a function that traverses any Expr and find declaration ref.

3. I believe that a MemberExpr has always a base. Similarly DeclRefExprs always have a referenced declaration (so you can omit the if).

I think you are right. Thanks!

[djtodoro]

• Handle all kinds of expressions when mark a param's modification

[riccibruno]

lib/Sema/SemaExpr.cpp

11292

Hmm, I don't think that this will work. Suppose that you have an expression like (a, b) += c You want to mark b as modified, but not a. What is needed I believe is:

Given an expression E find the variable that this expression designate if any, and if it is a function parameter mark it "known to be modified".

Visiting all of the children just looking for DeclRefExprs is a bit too blunt I believe.

11311

Hmm, we should avoid marking variables modification if this emits an error. But, we should emit it if CheckForModifiableLvalue returns false, since in the case of returning true an error will be emitted.

You are right, I believed for a moment that CheckForModifiableLvalue returned false on error.

[djtodoro]

lib/Sema/SemaExpr.cpp

11292

You are right. We thought it is possible implementing this with some basic analysis (with low overhead), but in order to support all cases it would take much time and it is not good idea.

I tried to use ExprMutationAnalyzer (that @lebedev.ri mentioned) and it seems to be useful for this. It seems that it can be used a little bit later than in this phase, because it takes ASTContext, but in this stage we have ASTContext finished up to node we are currently observing (an expression from which we are extracting DeclRef). But, even using it in the right time, it seems that ExprMutationAnalyzer does not have support for comma operator. I'm trying to extend this analyzer in order to add support for that. ASAP I will post new changes.
For the other cases I tried, it works.

[djtodoro]

-Rebase
-Use ExprMutationAnalyzer for parameter's modification check

[aprantl]

lib/CodeGen/CGDebugInfo.cpp

4516

Just looking at the type declarations in CGDebugInfo.h: Why not iterate over the SPCache directly? Shouldn't that contain all Function declarations only?

4520

clang-format please

4525

Could this be a cast<>?

[djtodoro]

lib/CodeGen/CGDebugInfo.cpp

4516

I tried it, but SPCache is empty at this point.

[djtodoro]

-Run clang-format
-Use cast instead of 'dyn_cast'

[djtodoro]

-Rebase

[aprantl]

lib/CodeGen/CGDebugInfo.cpp

3869

A dyn_cast followed by an unconditional use seems strange. I would expect either a cast or an if (PD) check.

[djtodoro]

lib/CodeGen/CGDebugInfo.cpp

3869

Yes, thanks for this!

[djtodoro]

-Careful use of dyn_cast
-Fill the ParamCache only in the case of EnableDebugEntryValues option

[aprantl]

lib/CodeGen/CGDebugInfo.cpp

3869

We shouldn't query CGM.getLangOpts().Optimize. If we don't want this to happen at -O0, we shouldn't set EnableDebugEntryValues at a higher level (Driver, CompilerInvocation, ...) ..Otherwise inevitably someone will query one but not the other and things will go out of sync.

4513

Please add either a top-level comment about what this block is doing or perhaps factor this out into a descriptively named static function.

4516

Where is it emptied? Just grepping through CGDebugInfo did not make this obvious to me.

[djtodoro]

lib/CodeGen/CGDebugInfo.cpp

3869

I agree. Thanks!

4513

Sure.

4516

The SPCache actually gets filled only in the case of CXXMemberFunction.
In the other cases of SP production there is only filling of DeclCache.
Should we use it like this or ?

[aprantl]

lib/CodeGen/CGDebugInfo.cpp

4516

If the number of entries in the DeclCache is much larger than the size of SPCache, we should keep them separate to speed up this loop. Otherwise we should join them to conserve memory.

[djtodoro]

-Use SPCache instead of DeclCache
-Refactor the code by addressing suggestions

[aprantl]

lib/CodeGen/CGDebugInfo.cpp

4441

/// Analyzes each function parameter to determine whether it is constant throughout the function body.

4451

if (FuncAnalyzer.isMutated(Parm))
  continue;

4456

should the else be an assertion or are there legitimate failure cases?

[aprantl]

Couple more nitpicks, but otherwise good.

[djtodoro]

-Add SPDefCache to speed up the process
-Add additional assertions that will improve quality of the code

[djtodoro]

-Rebase
-Add a test

include/clang/AST/Decl.h

enum TLSKind {
     TLS_Dynamic
   };
 
   /// Return the string used to specify the storage class \p SC.
   ///
   /// It is illegal to call this function with SC == None.
   static const char *getStorageClassSpecifierString(StorageClass SC);
 
+  mutable bool IsArgumentModified = false;
+
 protected:
   // A pointer union of Stmt * and EvaluatedStmt *. When an EvaluatedStmt, we
   // have allocated the auxiliary struct of information there.
   //
   // TODO: It is a bit unfortunate to use a PointerUnion inside the VarDecl for
   // this as *many* VarDecls are ParmVarDecls that don't have default
   // arguments. We could save some space by moving this pointer union to be
   // allocated in trailing space when necessary.
public:
   // Is this variable known to have a definition somewhere in the complete
   // program? This may be true even if the declaration has internal linkage and
   // has no definition within this source file.
   bool isKnownToBeDefined() const;
 
   /// Do we need to emit an exit-time destructor for this variable?
   bool isNoDestroy(const ASTContext &) const;
 
+  bool getIsArgumentModified() const {
+    return IsArgumentModified;
+  }
+  void setIsArgumentModified() const {
+    IsArgumentModified = true;
+  }
+
   // Implement isa/cast/dyncast/etc.
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) { return K >= firstVar && K <= lastVar; }
 };
 
 class ImplicitParamDecl : public VarDecl {
   void anchor() override;
 

lib/CodeGen/CGDebugInfo.cpp

if (!Unwritten) {
     Line = getLineNumber(VD->getLocation());
     Column = getColumnNumber(VD->getLocation());
   }
   SmallVector<int64_t, 13> Expr;
   llvm::DINode::DIFlags Flags = llvm::DINode::FlagZero;
   if (VD->isImplicit())
     Flags |= llvm::DINode::FlagArtificial;
 
+  auto &CGOpts = CGM.getCodeGenOpts();
+  if (CGOpts.EnableParamEntryValues && !VD->getIsArgumentModified())
+    Flags |= llvm::DINode::FlagArgumentNotModified;
+
+
   auto Align = getDeclAlignIfRequired(VD, CGM.getContext());
 
   unsigned AddressSpace = CGM.getContext().getTargetAddressSpace(VD->getType());
   AppendAddressSpaceXDeref(AddressSpace, Expr);
 
   // If this is implicit parameter of CXXThis or ObjCSelf kind, then give it an
   // object pointer flag.
   if (const auto *IPD = dyn_cast<ImplicitParamDecl>(VD)) {
auto *D = ArgNo ? DBuilder.createParameterVariable(
                                                 Flags, Align);
 
   // Insert an llvm.dbg.declare into the current block.
   DBuilder.insertDeclare(Storage, D, DBuilder.createExpression(Expr),
                          llvm::DebugLoc::get(Line, Column, Scope, CurInlinedAt),
                          Builder.GetInsertBlock());
 
   return D;
 }
 
 llvm::DILocalVariable *
 CGDebugInfo::EmitDeclareOfAutoVariable(const VarDecl *VD, llvm::Value *Storage,
                                        CGBuilderTy &Builder) {
   assert(DebugKind >= codegenoptions::LimitedDebugInfo);
   return EmitDeclare(VD, Storage, llvm::None, Builder);
 }
 
CGDebugInfo::getOrCreateNamespace(const NamespaceDecl *NSDecl) {
   NamespaceCache[NSDecl].reset(NS);
   return NS;
 }
 
 void CGDebugInfo::setDwoId(uint64_t Signature) {
   assert(TheCU && "no main compile unit");
   TheCU->setDWOId(Signature);
 }
 
 void CGDebugInfo::finalize() {
   // Creating types might create further types - invalidating the current
   // element and the size(), so don't cache/reference them.
   for (size_t i = 0; i != ObjCInterfaceCache.size(); ++i) {
     ObjCInterfaceCacheEntry E = ObjCInterfaceCache[i];
     llvm::DIType *Ty = E.Type->getDecl()->getDefinition()
                            ? CreateTypeDefinition(E.Type, E.Unit)
                            : E.Decl;
     DBuilder.replaceTemporary(llvm::TempDIType(E.Decl), Ty);
   }
 
   if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
     // Add methods to interface.
     for (const auto &P : ObjCMethodCache) {
       if (P.second.empty())
         continue;
 
       QualType QTy(P.first->getTypeForDecl(), 0);
       auto It = TypeCache.find(QTy.getAsOpaquePtr());
       assert(It != TypeCache.end());
 
       llvm::DICompositeType *InterfaceDecl =
           cast<llvm::DICompositeType>(It->second);
for (const auto &P : FwdDeclReplaceMap) {
     DBuilder.replaceTemporary(std::move(FwdDecl), cast<llvm::MDNode>(Repl));
   }
 
   // We keep our own list of retained types, because we need to look
   // up the final type in the type cache.
   for (auto &RT : RetainedTypes)
     if (auto MD = TypeCache[RT])
       DBuilder.retainType(cast<llvm::DIType>(MD));
 
   DBuilder.finalize();
 }
 
 void CGDebugInfo::EmitExplicitCastType(QualType Ty) {
   if (CGM.getCodeGenOpts().getDebugInfo() < codegenoptions::LimitedDebugInfo)
     return;
 
   if (auto *DieTy = getOrCreateType(Ty, TheCU->getFile()))
     // Don't ignore in case of explicit cast where it is referenced indirectly.
     DBuilder.retainType(DieTy);
 }
 
 llvm::DebugLoc CGDebugInfo::SourceLocToDebugLoc(SourceLocation Loc) {
   if (LexicalBlockStack.empty())
     return llvm::DebugLoc();
 
   llvm::MDNode *Scope = LexicalBlockStack.back();
   return llvm::DebugLoc::get(getLineNumber(Loc), getColumnNumber(Loc), Scope);
 }
 

lib/Sema/SemaExpr.cpp

 //===----------------------------------------------------------------------===//
 
 #include "TreeTransform.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTLambda.h"
 #include "clang/AST/ASTMutationListener.h"
 #include "clang/AST/CXXInheritance.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExprOpenMP.h"
 #include "clang/AST/RecursiveASTVisitor.h"
DiagnoseRecursiveConstFields(S, DRE->getDecl(), RTy, Loc,
             Range, OEK_Variable, DiagEmitted);
   else
     DiagnoseRecursiveConstFields(S, nullptr, RTy, Loc,
             Range, OEK_LValue, DiagEmitted);
   if (!DiagEmitted)
     DiagnoseConstAssignment(S, E, Loc);
 }
 
+/// Argument's value might be modified, so update the info.
+static void EmitArgumentsValueModification(Expr *E) {
+  if (const DeclRefExpr *LHSDeclRef =
+          dyn_cast<DeclRefExpr>(E->IgnoreParenCasts()))
+    if (const ValueDecl *Decl = LHSDeclRef->getDecl())
+      if (const VarDecl *VD = dyn_cast<VarDecl>(Decl)) {
+        if (!isa<ParmVarDecl>(VD))
+          return;
+        if (!VD->getIsArgumentModified())
+          VD->setIsArgumentModified();
+      }
+}
+
 /// CheckForModifiableLvalue - Verify that E is a modifiable lvalue.  If not,
 /// emit an error and return true.  If so, return false.
 static bool CheckForModifiableLvalue(Expr *E, SourceLocation Loc, Sema &S) {
   assert(!E->hasPlaceholderType(BuiltinType::PseudoObject));
 
   S.CheckShadowingDeclModification(E, Loc);
 
+  if (MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
+    if (Expr *BaseExpr = ME->getBase())
+      EmitArgumentsValueModification(BaseExpr);
+  } else
+    EmitArgumentsValueModification(E);
+
   SourceLocation OrigLoc = Loc;
   Expr::isModifiableLvalueResult IsLV = E->isModifiableLvalue(S.Context,
                                                               &Loc);
   if (IsLV == Expr::MLV_ClassTemporary && IsReadonlyMessage(E, S))
     IsLV = Expr::MLV_InvalidMessageExpression;
   if (IsLV == Expr::MLV_Valid)
     return false;
 
   unsigned DiagID = 0;
   bool NeedType = false;
   switch (IsLV) { // C99 6.5.16p2
   case Expr::MLV_ConstQualified:
     // Use a specialized diagnostic when we're assigning to an object
     // from an enclosing function or block.

test/CodeGen/debug-info-varchange.c

+// RUN: %clang -femit-param-entry-values -emit-llvm -S -g %s -o - | FileCheck %s
+
+// CHECK: !DILocalVariable(name: "a", arg: 1, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}}, flags: DIFlagArgumentNotModified)
+// CHECK: !DILocalVariable(name: "b", arg: 2, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+// CHECK: !DILocalVariable(name: "test_s", arg: 3, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}}, flags: DIFlagArgumentNotModified)
+// CHECK: !DILocalVariable(name: "test_s2", arg: 4, scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+// CHECK: !DILocalVariable(name: "x", scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+// CHECK: !DILocalVariable(name: "y", scope: {{.*}}, file: {{.*}}, line: {{.*}}, type: {{.*}})
+
+typedef struct s {
+  int m;
+  int n;
+}S;
+
+void foo (int a, int b,
+          S test_s, S test_s2)
+{
+  b++;
+  b = a + 1;
+  if (b>4)
+    test_s2.m = 434;
+}
+
+int main()
+{
+  S test_s = {4, 5};
+
+  int x = 5;
+  int y = 6;
+
+  foo(x , y, test_s, test_s);
+
+  return 0;
+}
+