Add checks when default intents allow indirect modification (chapel-l…

…ang#23952)

[reviewed by @mppf]

Similar to chapel-lang#23885, this causes the blank intent to generate warnings
when the actual argument intent ends up being `const ref` because of the
argument's type.

<details>

I moved the `unstable` check during resolveIntents into the now-renamed
function so that it was sharable by both checks instead of duplicated.
Thankfully, the warning message was general enough that I didn't need
any specialization for it in the blank intent case.

</details>

There were additional changes needed to fix some issues pointed out by
valgrind with the check in general (regardless of if the `const` intent
or default intent was used). Specifically records, strings, and some
iterators were causing warnings about accessing uninitialized memory. To
rectify that, explicitly zero the memory in these cases but only when
the check will be inserted.

Will look into warning for arrays passed by default/const intent when
the domain is changed indirectly next

Modified the blank intent test for the new expected output. Added
several tests while locking down exactly what was triggering the
valgrind failures.

Passed a full paratest with futures and a full valgrindexe run.

compiler/AST/iterator.cpp

@@ -1847,6 +1847,12 @@ rebuildIterator(IteratorInfo* ii,
 
   fn->insertAtTail(new DefExpr(iterator));
 
+  // Avoids a valgrind warning about uninitialized memory when performing
+  // indirect modification checks on const and default intent arguments
+  if (fWarnUnstable && !fNoConstArgChecks) {
+    fn->insertAtTail(new CallExpr(PRIM_ZERO_VARIABLE, new SymExpr(iterator)));
+  }
+
   // For each live argument
   forv_Vec(Symbol, local, locals) {
     if (!toArgSymbol(local))

compiler/main/driver.cpp

@@ -2307,6 +2307,7 @@ static chpl::CompilerGlobals dynoBuildCompilerGlobals() {
   return {
     .boundsChecking = !fNoBoundsChecks,
     .castChecking = !fNoCastChecks,
+    .constArgChecking = !fNoConstArgChecks,
     .nilDerefChecking = !fNoNilChecks,
     .overloadSetsChecking = fOverloadSetsChecks,
     .divByZeroChecking = !fNoDivZeroChecks,

compiler/passes/resolveIntents.cpp

@@ -254,12 +254,12 @@ IntentTag blankIntentForExternFnArg(Type* type) {
     return INTENT_CONST_IN;
 }
 
-// Add calls to some runtime functions used to determine if a `const` argument
-// that would be transformed into `const ref` is implicitly modified over the
-// course of the function.  We may decide to change this behavior in the future,
-// so want to generate a warning if it occurs (and one of the primitives will
-// handle generating that warning).
-static void warnForConstIntent(ArgSymbol* arg) {
+// Add calls to some runtime functions used to determine if a `const` or blank
+// intent argument that would be transformed into `const ref` is implicitly
+// modified over the course of the function.  We may decide to change this
+// behavior in the future, so want to generate a warning if it occurs (and one
+// of the primitives will handle generating that warning).
+static void warnForInferredConstRef(ArgSymbol* arg) {
   SET_LINENO(arg);
 
   FnSymbol* fn = toFnSymbol(arg->defPoint->parentSymbol);
@@ -271,38 +271,51 @@ static void warnForConstIntent(ArgSymbol* arg) {
     return;
   }
 
-  // Hash the argument at the start of the function
-  CallExpr* getStartHash = new CallExpr(PRIM_CONST_ARG_HASH, new SymExpr(arg));
-
-  VarSymbol* startHash = newTemp(dtUInt[INT_SIZE_64]);
-  DefExpr* startDef = new DefExpr(startHash);
-
-  CallExpr* outerStart = new CallExpr(PRIM_MOVE, new SymExpr(startHash),
-                                      getStartHash);
-
-  fn->insertAtHead(outerStart);
-  outerStart->insertBefore(startDef);
-
-  // Hash the argument at the end of the function
-  CallExpr* getEndHash = new CallExpr(PRIM_CONST_ARG_HASH, new SymExpr(arg));
-
-  VarSymbol* endHash = newTemp(dtUInt[INT_SIZE_64]);
-  DefExpr* endDef = new DefExpr(endHash);
-
-  CallExpr* outerEnd = new CallExpr(PRIM_MOVE, new SymExpr(endHash),
-                                    getEndHash);
-
-  // Create defer block to ensure we will always check the end hash
-  DeferStmt* finishCheck = new DeferStmt(outerEnd);
-  outerEnd->insertBefore(endDef);
-
-  // Ensure the two hashes match.  If not, will generate a runtime error
-  CallExpr* checkHash = new CallExpr(PRIM_CHECK_CONST_ARG_HASH,
-                                     new SymExpr(startHash),
-                                     new SymExpr(endHash),
-                                     new_CStringSymbol(arg->name));
-  outerEnd->insertAfter(checkHash);
-  outerStart->insertAfter(finishCheck);
+  // Want this to be an unstable warning, and not trigger for internal or
+  // standard modules unless the appropriate flag is used
+  auto mod = fn->getModule();
+  bool shouldWarnInternal = (mod->modTag == MOD_INTERNAL &&
+                             fWarnUnstableInternal);
+  bool shouldWarnStandard = (mod->modTag == MOD_STANDARD &&
+                             fWarnUnstableStandard);
+  if (fWarnUnstable && (shouldWarnInternal || shouldWarnStandard ||
+                        mod->modTag == MOD_USER) &&
+      !fNoConstArgChecks) {
+
+    // Hash the argument at the start of the function
+    CallExpr* getStartHash = new CallExpr(PRIM_CONST_ARG_HASH,
+                                          new SymExpr(arg));
+
+    VarSymbol* startHash = newTemp(dtUInt[INT_SIZE_64]);
+    DefExpr* startDef = new DefExpr(startHash);
+
+    CallExpr* outerStart = new CallExpr(PRIM_MOVE, new SymExpr(startHash),
+                                        getStartHash);
+
+    fn->insertAtHead(outerStart);
+    outerStart->insertBefore(startDef);
+
+    // Hash the argument at the end of the function
+    CallExpr* getEndHash = new CallExpr(PRIM_CONST_ARG_HASH, new SymExpr(arg));
+
+    VarSymbol* endHash = newTemp(dtUInt[INT_SIZE_64]);
+    DefExpr* endDef = new DefExpr(endHash);
+
+    CallExpr* outerEnd = new CallExpr(PRIM_MOVE, new SymExpr(endHash),
+                                      getEndHash);
+
+    // Create defer block to ensure we will always check the end hash
+    DeferStmt* finishCheck = new DeferStmt(outerEnd);
+    outerEnd->insertBefore(endDef);
+
+    // Ensure the two hashes match.  If not, will generate a runtime error
+    CallExpr* checkHash = new CallExpr(PRIM_CHECK_CONST_ARG_HASH,
+                                       new SymExpr(startHash),
+                                       new SymExpr(endHash),
+                                       new_CStringSymbol(arg->name));
+    outerEnd->insertAfter(checkHash);
+    outerStart->insertAfter(finishCheck);
+  }
 }
 
 IntentTag concreteIntentForArg(ArgSymbol* arg) {
@@ -326,25 +339,20 @@ IntentTag concreteIntentForArg(ArgSymbol* arg) {
     return INTENT_REF;
 
   else {
+    // Lydia 11/29/23 TODO: use `ArgSymbol->originalIntent` and check this
+    // in a later pass that does more AST transformation.  Potentially
+    // callDestructors?
     if (arg->intent == INTENT_CONST) {
       // No need to warn if the argument intent was going to be converted to
       // `const in`
       if (constIntentForType(arg->type) == INTENT_CONST_REF) {
-        // Want this to be an unstable warning, and not trigger for internal or
-        // standard modules
-        auto mod = fn->getModule();
-        bool shouldWarnInternal = (mod->modTag == MOD_INTERNAL &&
-                                   fWarnUnstableInternal);
-        bool shouldWarnStandard = (mod->modTag == MOD_STANDARD &&
-                                   fWarnUnstableStandard);
-        if (fWarnUnstable && (shouldWarnInternal || shouldWarnStandard ||
-                              mod->modTag == MOD_USER) &&
-            !fNoConstArgChecks) {
-          // Lydia 11/29/23 TODO: use `ArgSymbol->originalIntent` and check this
-          // in a later pass that does more AST transformation.  Potentially
-          // callDestructors?
-          warnForConstIntent(arg);
-        }
+        warnForInferredConstRef(arg);
+      }
+    } else if (arg->intent == INTENT_BLANK) {
+      // Only warn if the argument intent was going to be converted to `const
+      // ref`
+      if (blankIntentForType(arg->type) == INTENT_CONST_REF) {
+        warnForInferredConstRef(arg);
       }
     }
     return concreteIntent(arg->intent, arg->type);

compiler/resolution/functionResolution.cpp

@@ -3836,7 +3836,6 @@ void resolveNormalCallAdjustAssign(CallExpr* call) {
   }
 }
 
-
 static
 FnSymbol* resolveNormalCall(CallExpr* call, check_state_t checkState) {
   CallInfo  info;

compiler/resolution/initializerResolution.cpp

@@ -342,6 +342,17 @@ static CallExpr* buildInitCall(CallExpr* newExpr,
     USR_PRINT(call, "init resulted in type '%s'", toString(tmp->type));
   }
 
+  // Avoid a potential access of uninitialized memory in the case where the
+  // record has a bool field.  C will pad around boolean fields, but that memory
+  // cannot normally be updated - without explicitly zero initializing it,
+  // valgrind will complain when we use it to create the hash to ensure inferred
+  // const ref arguments are not implicitly modified.
+  if (isRecord(tmp->type)) {
+    if (fWarnUnstable && !fNoConstArgChecks) {
+      call->insertBefore(new CallExpr(PRIM_ZERO_VARIABLE, new SymExpr(tmp)));
+    }
+  }
+
   return call;
 }
 

frontend/include/chpl/uast/compiler-globals-list.h

@@ -19,6 +19,7 @@
 
 COMPILER_GLOBAL(bool, "boundsChecking", boundsChecking)
 COMPILER_GLOBAL(bool, "castChecking", castChecking)
+COMPILER_GLOBAL(bool, "chpl_constArgChecking", constArgChecking)
 COMPILER_GLOBAL(bool, "chpl_checkNilDereferences", nilDerefChecking)
 COMPILER_GLOBAL(bool, "chpl_overloadSetsChecks", overloadSetsChecking)
 COMPILER_GLOBAL(bool, "chpl_checkDivByZero", divByZeroChecking)

modules/internal/String.chpl

@@ -700,6 +700,16 @@ module String {
       // same names, because "wellknown" implementation in the compiler does not
       // allow overloads.
       var ret: string;
+
+      // 2024/02/15 Lydia NOTE: This avoids a valgrind warning when performing
+      // checks about arguments passed by default/const intent when they could
+      // be implicitly modified.  The string type has boolean fields, which C
+      // inserts padding for but we can't actually initialize the padding
+      // without a memset
+      if (chpl_warnUnstable && chpl_constArgChecking) {
+        __primitive("zero variable", ret);
+      }
+
       initWithBorrowedBuffer(ret, x, length, size);
       ret.cachedNumCodepoints = numCodepoints;
       return ret;

test/unstable/const-intent/arrayField.chpl

@@ -0,0 +1,17 @@
+record Foo {
+  var x: [0..3] int;
+}
+
+var arr = [2, 4, 6, 2];
+var globalFoo = new Foo(arr);
+
+proc takesAFoo(const arg: Foo) {
+  writeln(arg);
+  // Won't trigger the warning
+  globalFoo.x[2] = 7;
+  writeln(arg);
+}
+
+proc main() {
+  takesAFoo(globalFoo);
+}

test/unstable/const-intent/arrayField.good

@@ -0,0 +1,2 @@
+(x = 2 4 6 2)
+(x = 2 4 7 2)

test/unstable/const-intent/boolField.chpl

@@ -0,0 +1,16 @@
+record Foo {
+  var x: bool = false;
+}
+
+var globalFoo = new Foo(true);
+
+proc takesAFoo(const arg: Foo) {
+  writeln(arg);
+  // This should trigger the warning, but not on this line number
+  globalFoo.x = false;
+  writeln(arg);
+}
+
+proc main() {
+  takesAFoo(globalFoo);
+}

test/unstable/const-intent/boolField.good

@@ -0,0 +1,4 @@
+(x = true)
+(x = false)
+boolField.chpl:7: warning: The argument 'arg' was modified indirectly during this function, this behavior is unstable and may change in the future.
+If this behavior is intentional, declaring the argument as 'const ref' will silence this warning.  If not, declaring the argument as 'const in' will prevent indirect modifications

test/unstable/const-intent/boolFieldNoDefaultValue.chpl

@@ -0,0 +1,16 @@
+record Foo {
+  var x: bool;
+}
+
+var globalFoo = new Foo(true);
+
+proc takesAFoo(const arg: Foo) {
+  writeln(arg);
+  // This should trigger the warning, but not on this line number
+  globalFoo.x = false;
+  writeln(arg);
+}
+
+proc main() {
+  takesAFoo(globalFoo);
+}

test/unstable/const-intent/boolFieldNoDefaultValue.good

@@ -0,0 +1,4 @@
+(x = true)
+(x = false)
+boolFieldNoDefaultValue.chpl:7: warning: The argument 'arg' was modified indirectly during this function, this behavior is unstable and may change in the future.
+If this behavior is intentional, declaring the argument as 'const ref' will silence this warning.  If not, declaring the argument as 'const in' will prevent indirect modifications

test/unstable/const-intent/defaultIntentMod.good

@@ -1,2 +1,4 @@
 (x = 15)
 (x = 3)
+defaultIntentMod.chpl:7: warning: The argument 'r' was modified indirectly during this function, this behavior is unstable and may change in the future.
+If this behavior is intentional, declaring the argument as 'const ref' will silence this warning.  If not, declaring the argument as 'const in' will prevent indirect modifications

test/unstable/const-intent/recordWithRecordFieldWithBoolField.chpl

@@ -0,0 +1,20 @@
+record Bar {
+  var x: bool = false;
+}
+
+record Foo {
+  var x: Bar;
+}
+
+var globalFoo = new Foo(new Bar(true));
+
+proc takesAFoo(const arg: Foo) {
+  writeln(arg);
+  // This should trigger the warning, but not on this line number
+  globalFoo.x.x = false;
+  writeln(arg);
+}
+
+proc main() {
+  takesAFoo(globalFoo);
+}

test/unstable/const-intent/recordWithRecordFieldWithBoolField.good

@@ -0,0 +1,4 @@
+(x = (x = true))
+(x = (x = false))
+recordWithRecordFieldWithBoolField.chpl:11: warning: The argument 'arg' was modified indirectly during this function, this behavior is unstable and may change in the future.
+If this behavior is intentional, declaring the argument as 'const ref' will silence this warning.  If not, declaring the argument as 'const in' will prevent indirect modifications

test/unstable/const-intent/recordWithRecordFieldWithLaterBoolField.chpl

@@ -0,0 +1,21 @@
+record Bar {
+  var w: int = 5;
+  var x: bool;
+}
+
+record Foo {
+  var x: Bar;
+}
+
+var globalFoo = new Foo(new Bar(3, true));
+
+proc takesAFoo(const arg: Foo) {
+  writeln(arg);
+  // This should trigger the warning, but not on this line number
+  globalFoo.x.x = false;
+  writeln(arg);
+}
+
+proc main() {
+  takesAFoo(globalFoo);
+}

test/unstable/const-intent/recordWithRecordFieldWithLaterBoolField.good

@@ -0,0 +1,4 @@
+(x = (w = 3, x = true))
+(x = (w = 3, x = false))
+recordWithRecordFieldWithLaterBoolField.chpl:12: warning: The argument 'arg' was modified indirectly during this function, this behavior is unstable and may change in the future.
+If this behavior is intentional, declaring the argument as 'const ref' will silence this warning.  If not, declaring the argument as 'const in' will prevent indirect modifications