Problem with runtime type and default initialization

[mppf]

After PR #15869, a problem came up in nightly testing that can be reproduced by the following code:

record R {
  type t;
  var x: int;
  proc init(type t, in arg: int = 1) {
    this.t = t;
    this.x = arg;
  }
}

proc main() {
  var A:[1..3] int;
  var rr: R(A.type);

  var B:rr.t;
  writeln(B);
}

What happens is that the access rr.t ends up pointing to uninitialized memory. This is because the compiler computes the type expression R(A.type) separately from initializing rr. In particular, in createGenericRecordVarDefaultInitCall, it creates an uninitialized temporary storing the runtime type (see the code creating a variable named default_runtime_temp and the related comment).

A workaround is to use e.g. var rr = new R(A.type).

I can see two ways we might be able to fix the bug:

1. Associate runtime types of fields with runtime types of aggregates. For example, the code could have been type t = R(A.type); var rr: t and in that event the only way to get the value A.type at runtime into rr is if the t variable stores the runtime type for its field. This might be relatively straightforward for records but might become untenable or impossible to do with classes. (But maybe that is a restriction we could document). This has been a long-term todo - see handle record and class types with runtime types #11549.
2. Adjust the compiler so that it can handle the runtime types in the declaration var rr: R(A.type); more directly. In particular, this might involve adjusting PRIM_DEFAULT_INIT_VAR to accept extra arguments for runtime types of fields.

[daviditen]

Unfortunately the workaround isn't working for maps of classes. After doing the workaround on the map.table field, I'm seeing internal compiler errors for this code:

use Map;

class C {
  var i: int;
}

var m = new map(int, owned C?);
m.add(1, new owned C?(1));

$CHPL_HOME/modules/standard/Map.chpl:74: internal error: invalid attempt to get reference type [AST/primitive.cpp:235]

With this diff from master:

diff --git a/modules/standard/Map.chpl b/modules/standard/Map.chpl
index 9f1a67136e..29b4676628 100644
--- a/modules/standard/Map.chpl
+++ b/modules/standard/Map.chpl
    param parSafe = false;

    pragma "no doc"
-   var table: chpl__hashtable(keyType, valType);
+   var table = new chpl__hashtable(keyType, valType);

    pragma "no doc"
    var _lock$ = if parSafe then new _LockWrapper() else none;

[mppf]

@daviditen - I'd suspect that is a different issue but I'm not seeing this problem with revision 27f25b4 and your patch applied on my system.

[daviditen]

Strange. I'm currently fully updated and see it on my Mac laptop, chapcs11, and the travis build of mason for PR #15932 .

[mppf]

@daviditen - I've checked out your branch on chapcs11 and I'm not seeing the same issue. Is it possible that there is an error in the test program pasted above? Works for me on my Mac too.

and the travis build of mason for PR #15932 .

I do see the error when building mason. Here it looks like the problem is that the compiler is trying to resolve PRIM_ADDR_OF with a chpl__hashtable variable that has generic type. I'm pretty sure that is a separate error and I would appreciate it if you are able to make a small reproducer and issue.

I went ahead and tried a different workaround in Map. I applied this patch (to your branch):

diff --git a/modules/standard/Map.chpl b/modules/standard/Map.chpl
index 9f1a67136e..343469ca6e 100644
--- a/modules/standard/Map.chpl
+++ b/modules/standard/Map.chpl
@@ -71,7 +71,7 @@ module Map {
     param parSafe = false;
 
     pragma "no doc"
-    var table = new chpl__hashtable(keyType, valType);
+    var table: chpl__hashtable(keyType, valType);
 
     pragma "no doc"
     var _lock$ = if parSafe then new _LockWrapper() else none;
@@ -117,6 +117,7 @@ module Map {
       this.keyType = keyType;
       this.valType = valType;
       this.parSafe = parSafe;
+      this.table = new chpl__hashtable(keyType, valType);
     }
 
     proc init(type keyType, type valType, param parSafe=false)
@@ -143,6 +144,7 @@ module Map {
       this.keyType = keyType;
       this.valType = valType;
       this.parSafe = parSafe;
+      this.table = new chpl__hashtable(keyType, valType);
     }
 
     /*
@@ -164,6 +166,7 @@ module Map {
       this.keyType = kt;
       this.valType = vt;
       this.parSafe = ps;
+      this.table = new chpl__hashtable(keyType, valType);
 
       this.complete();
 

and after that, mason builds for me.

[daviditen]

Huh. Somehow the code from my comment is passing for me too now. I guess I copied the wrong thing. Sorry.

I would appreciate it if you are able to make a small reproducer and issue.

test/library/standard/Map/testBorrowedMap.chpl is about the best I've been able to do so far. I tried to tease it out of map, but without luck. I opened issue #15960.

and after that, mason builds for me.

But then we're back into the problem this issue is about and optimizations/remoteValueForwarding/serialization/globals.chpl and optimizations/remoteValueForwarding/serialization/staticSize.chpl fail again.

[bradcray]

@mppf: I just slammed back into this while trying to write a new test to lock something in and took an embarrassingly long time to debug it given that I've been working around it for the past few workdays (though I guess it is a Monday). Anyway, it made me wonder what we might be able to do to catch and complain about such cases rather than having them go through and result in unexpected behavior. But so far all I've done is wonder rather than actually think about it, so I'm putting this here to see what it jogs for you or remind myself to think about it more.

[mppf]

I wouldn't expect it to be hard to emit an error or warning for it. The things that seem hard to me are to get it working comprehensively in all of these cases.

[mppf]

@bradcray - here is a patch that adds a warning for the whole block of code that is problematic. We see the warning for the test in this issue but not for hello world. A reasonable next step would be to run testing with it to see if we have cases relying on it today. (I suspect that there used to be, especially within the array implementation, but that these have been gradually migrated away from this pattern).

diff --git a/compiler/resolution/functionResolution.cpp b/compiler/resolution/functionResolution.cpp
index cc01b21d7d..ac26be7a31 100644
--- a/compiler/resolution/functionResolution.cpp
+++ b/compiler/resolution/functionResolution.cpp
@@ -11409,6 +11409,7 @@ static CallExpr* createGenericRecordVarDefaultInitCall(Symbol* val,
       appendExpr = new SymExpr(value);
     } else if (field->hasFlag(FLAG_TYPE_VARIABLE)) {
       if (value->getValType()->symbol->hasFlag(FLAG_HAS_RUNTIME_TYPE)) {
+        USR_WARN(call, "Problematic default init of runtime type");
         // BHARSH 2018-11-02: This technically generates code that would
         // crash at runtime because aggregate types don't contain the runtime
         // type information for their fields, so this temporary will go

[bradcray]

OK, thanks! I suspect I can run a paratest against this this afternoon if you don't get to it first.

[bradcray]

The result was 29 failures, the vast majority of which had the output:

> $CHPL_HOME/modules/internal/ChapelBase.chpl:897: warning: Problematic default initialization of runtime type
> $CHPL_HOME/modules/internal/ChapelBase.chpl:903: warning: Problematic default initialization of runtime type
> $CHPL_HOME/modules/internal/DefaultRectangular.chpl:1504: warning: Problematic default initialization of runtime type
> $CHPL_HOME/modules/internal/DefaultRectangular.chpl:967: warning: Problematic default initialization of runtime type

(where the main other cases were tests that created sync/single of array or the like—not currently supported, but generate the error).

I haven't dug into this at all yet, but with this quick look wonder whether—if we were to replace the immediate warning with an insertion of a compilerError() (or warning) would determine that the code paths in question are never used. If so, we could either stick with the compilerError() or we could just squash the warning for any non-user modules.

[mppf]

I suppose that those errors are indicating that there's probably code you could write with array-of-record-containing-array that would lead to memory errors. Perhaps we could/should silence the warning for internal modules in the near term though.

[dlongnecke-cray]

Just ran into this again in #19118.

For the pattern var obj: foo(domain(3)), I think we are constructing the runtime type, it's just that we discard it when we resolve foo(domain(3)) by replacing that expression with a SymExpr pointing to a type. So I think this might be a case of discarding info too early when an actual in the type construction call is a runtime type.

This doesn't happen in new foo(domain(3)) because we correctly pass the constructed runtime type into the init call (and then resolve that init call all at once, I'd assume).

For the code:

record foo {
  type T;
  proc doSomething(in val: T) {
    writeln(val);
  }
}

proc test() {
  var obj: foo(domain(3));
  obj.doSomething({0..1, 2..3, 4..5});
}
test();

Coming at you with DavidL levels of excessive detail, now, because I need input on my idea.

The logged AST before resolve looks like this:

unknown obj[200288] "dead at end of block" "insert auto destroy"
unknown call_tmp[723085] "expr temp" "maybe param" "maybe type" "temp"

// Here's the runtime type we construct for `domain(3)`
(723088 'move' call_tmp[723085](200284 call chpl__buildDomainRuntimeType defaultDist[92798] 3))
unknown call_tmp[723090] "expr temp" "maybe param" "maybe type" "temp"

// The inner expression in the move is the foo(domain(3)) part of the record declaration in 'var obj: foo(domain(3))'
(723093 'move' call_tmp[723090](200287 call foo[200269](?) call_tmp[723085]))
(555989 'default init var' obj[200288] call_tmp[723090])

When the compiler goes to resolve (200287 call foo[200269](?) call_tmp[723085]), the foo(domain(3)) type specifier, it opts to replace that entire call with a SymExpr pointing to the instantiated record type.

This is what causes the runtime type to be lost.

I think we could opt to not postfold these type instantiation calls at all, or conditionally not fold them if they contain a runtime type. Either way would work, I think?

When resolving the default-init we can consult the call and compute the type symbol later if we need to in the same fashion as postfold.

This would allow the default-init to just use the runtime type as it appears in the code instead of constructing an empty temp.

Thoughts?

Another option would be to walk backwards from the default-init and look for a specific pattern, but I am less confident about that idea.

[dlongnecke-cray]

So I ran with my idea a bit and came up with something that actually works for my test case above, but day22a-listOfDomain.chpl is exploding due to problems in pass parallel.

As I was working on this, I kept asking "why do we replace type constructor calls with SymExprs to the type at all"? It seems like the situation would be handled naturally if we just didn't do that, because for foo(domain(3)) you can still get the fully resolved type from the baseExpr of the call. So I'm increasingly feeling this is a situation where we just replace nodes a bit too eagerly when resolving (it's not actually the normalizer's fault this time!).

(Well, it's also kind of the normalizer's fault, because the situation would be even better if we could just see (PRIM_DEFAULT_INIT_VAR obj (call foo domain(3))) without any temps in there...)