bundle reflection should not get width as data field

[shunshou]

Fixes #454 thx to @sdtwigg :)

[shunshou]

@ducky64 care to review + merge (or come up with a different solution)? Would like to just get this out of the way...

[ducky64]

I'm going to do a bit more testing. From what I traced through the code this may be indicative of a larger problem:

• getPublicFields (Builder.scala) is designed to ignore methods declared below Bundle (so Data.width should be hidden).
• Your declaration of width: Width seems to be confusing things. If your custom width is overriding Data.width, then that needs to be fixed to be impossible (or at least throw a elaboration-time error), otherwise things like fromBits/asUInt will malfunction and can create difficult-to-trace (won't error out!) bugs. Otherwise, if it's invoking the wrong width, then getPublicFields needs to be fixed.
• Overall, your solution appears correct, but it seems to be patching things that shouldn't happen in the first place.

[shunshou]

FYI, a (temporary) patch is better for the end user than broken code, so it's quite frustrating that something like this must sit around until a better solution happens... (who knows when). --> Fork!

[ducky64]

Working through it now, hopefully will have an alternative PR up by end of day.

The problem seems to be getPublicFields is filtering on methods based on method name, but your width (constructor parameter) is being promoted to a field as it is used in cloneType (https://stackoverflow.com/questions/11852950/cannot-use-getdeclaredfields-to-retrieve-fields-of-a-scala-class), causing a false positive in getPublicFields. Looking into solutions...

[sdtwigg]

This is a case of having to work around edge cases in how scalac actually generates the underlying java classes and more specifically, working around the tricks it uses to add features that java doesn't have. (Keep in mind that java doesn't have the finer grained access modifiers of scala, compiler-enforced UAP, "body is constructor" semantics, closure rules, etc.) As far as I know, there is no well-defined spec for this since it is a moving target across JVM and scala major versions.

The underlying scenario is that getDeclaredFields returns all the fields in the java class. For a scala val in the class body, scalac will reliably (as it kind of has to in order to have stable storage) create a corresponding private field in the class and then a getter for it. The getter (to maintain a semblance of java interop) will have the same name as the scala val identifier. We are fortunate in that (currently always in all cases we have seen) the underlying field in the java class has the same name as the getter.

However, scalac needs to make other private fields in that java class to implement other scala features, like closures from the constructor args into class methods. However, because these fields are being used to implement closures inside the class's own methods, there is no need to generate a getter. This explains why we usually do not see these fields in the reflection.

In this case, however, there is a 'pseudo-private' (package private in scala but I think public in java since java won't let you say an item is private to an enclosing package) 0-arg method called 'width' in Data. The subclass constructor also takes an argument called width, which it closes on and must store (privately). We then end up with a private field width and a public function width that thus looks like a getter for that field (even though both are unrelated).

Calling that then leads to a stack-overflow induced by an infinite loop (width asks for elements, which invokes width again, etc.) Now the matter becomes how to avoid this case. (And really, because of the arcane nature of which scalac generates java classes is more of an art/whack-a-mole problem than exact science. We could reverse engineer scalac but there is no guarantee they won't change the finer points of java interop in later versions. Keep in mind they definitely did this between 2.11 and 2.12, for example.)

Candidates were:

• check that the field and getter matches return type: (fails in this case because both happen to be Width)
• check the modifiers of the getter to ensure public: (fails in this case because scalac erases some of that modifier information when going into java land)
• check that the getter wasn't mysteriously coming from Bundle or some parent (chosen because it was clear width() was coming from Data)

There are likely other ways to analyze and filter the methods/fields (java reflection has a plethora of other functions to play with) but they are all still best guesses of what scalac is actually doing. (None of them are called '.isGetter' because that concept doesn't really exist in base java classes and we can't safely call all single-argument functions.)

[ducky64]

It seems that a solution using the Scala reflection API (rather than the Java reflection API) might be better, like https://stackoverflow.com/questions/31324664/how-can-i-get-all-non-final-object-vals-and-subobject-vals-using-reflection-in
(but with filtering based on declared class, so we ignore things in Bundle and superclasses)

[sdtwigg]

We initially experimented internally with an scala reflection (I actually still have the CL where I tried it hanging around in a zombie state). It worked on the unit tests and then failed catastrophically at other use cases: The scala.reflect threw an internal state corruption error deep in the internals rooted in an unrelated user-code source line (see PS). It is unclear exactly why (it is possible that scala.reflect itself is just fragile or the way our build system handles dependencies caused the resultant jars to drop some of the scala metadata).

It was actually suggestions from @jsuereth to look into using java reflection as java's reflect api is substantially more reliable (albeit with the caveat that it can't see everything scala does). Also, the scala.reflect API is marked as experimental and, according to the 2.12 release notes, they are not shy about changing it.

PS: I just re-ran the example. scala.reflect got really confused on an anonymous class caused due to a refinement on Bundle that added fields).

[ducky64]

Is there a minimal test case that could be added into the standard chisel regressions? It would be worth figuring out exactly what's failing and possibly bypassing that - for example, if we can still do:

im.symbol.asClass.typeSignature.members filter (s => s.isTerm && s.asTerm.isAccessor)

and convert the result to a string, that might still be better than a getDeclaraedFields check.

If Scala reflection is a non-starter, then as far as this PR goes it may be worth putting the check in chiselFrontend/src/main/scala/chisel3/internal/Builder.scala, replacing isPublicVal in getPublicFields:

def isPublicVal(m: java.lang.reflect.Method) =
  m.getParameterTypes.isEmpty && valNames.contains(m.getName) && m.getDeclaringClass.isAssignableFrom(rootClass)

since getPublicFields is also used in Module, which may be vulnerable to a similar bug. But this kind of feels like a game of whackamole.

Also,

private[core] object Bundle {
  val keywords = List("flip", "asInput", "asOutput", "cloneType", "chiselCloneType", "toBits",
    "widthOption", "signalName", "signalPathName", "signalParent", "signalComponent")
}

in Aggregate.scala should be removed, it's dead code.

[sdtwigg]

I think it is a bit odd that the reflection functions are in Builder (and inherited by almost all chisel) and not some reflection utilities that only Bundle and Module call.

I had done experimenting with the scala apis here (internal version applied to chisel is basically the same): https://github.com/sdtwigg/gama/blob/master/chiselfrontend/src/main/scala/frontend/implementation/Reflection.scala#L20
I can't remember at the moment why I still had to do the name matching; however, I do know some of the fields do have stuff like isVal and isGetter, which are supposed to be more precise than isAccessor. Regardless, the exception issues with scala.reflect (on top of it being marked experimental and specifically called out in the changelog as unstable) make me reluctant to proceed with that solution.

Regardless, this discussion has veered into being about a cleanup refactoring and somewhat out of scope for fixing the bug at hand here.

[ducky64]

I think this is fine if it gets moved into Builder.

Removing the unused keywords from Aggregate.scala is nice but not required. All the other discussions are probably way out of scope.

[ducky64]

Move this into Builder's getPublicFields, so the entire class of bugs is stomped out (rather than just for Bundles)

[shunshou]

So should I just do what you said earlier? i.e.

def isPublicVal(m: java.lang.reflect.Method) =
  m.getParameterTypes.isEmpty && valNames.contains(m.getName) && m.getDeclaringClass.isAssignableFrom(rootClass)

[ducky64]

Yeah, so it solves the problem (within the limits of the current architecture) at the root, and so the fix also applies for Module reflection.

[shunshou]

So I just got to trying what you suggested and no tests pass (compiles OK, but runtime derps):

[info]   chisel3.core.Binding$BindingException: 'this' (chisel3.core.UInt@7): Not bound to synthesizable node, currently only Type description
[info]   at chisel3.core.Binding$.checkSynthesizable(Binding.scala:186)
[info]   at chisel3.core.Bits.do_apply(Bits.scala:100)
[info]   at chisel3.core.Bits.do_apply(Bits.scala:112)
[info]   at chiselTests.CompatibiltySpec$$anonfun$13$RequireIOWrapModule$1.<init>(CompatibiltySpec.scala:206)
[info]   at chiselTests.CompatibiltySpec$$anonfun$13$$anonfun$apply$mcV$sp$8.apply(CompatibiltySpec.scala:208)
[info]   at chiselTests.CompatibiltySpec$$anonfun$13$$anonfun$apply$mcV$sp$8.apply(CompatibiltySpec.scala:208)
[info]   at chisel3.core.Module$.do_apply(Module.scala:42)
[info]   at chisel3.Driver$$anonfun$elaborate$1.apply(Driver.scala:91)

info] - Bundles with same data but different, underlying elements should compare as UInt *** FAILED ***
[info]   chisel3.core.Binding$BindingException: 'this' (chisel3.core.UInt@a): Not bound to synthesizable node, currently only Type description
[info]   at chisel3.core.Binding$.checkSynthesizable(Binding.scala:186)
[info]   at chisel3.core.Data.connect(Data.scala:186)
[info]   at chisel3.core.Data.$colon$eq(Data.scala:258)
[info]   at chiselTests.BundleToUnitTester.<init>(BundleWire.scala:38)
[info]   at chiselTests.BundleToUIntSpec$$anonfun$3$$anonfun$apply$mcV$sp$2.apply(BundleWire.scala:70)
[info]   at chiselTests.BundleToUIntSpec$$anonfun$3$$anonfun$apply$mcV$sp$2.apply(BundleWire.scala:70)

Pushed changes to https://github.com/ucb-bar/chisel3/commits/builderreflectionfix if you want to take a look. If I'm not doing something stupid (copy pasting incorrectly), then maybe it's better to just consider merging this PR... unless you care to figure out what's going on, b/c reflection stuff is kind of over my head.

[shunshou]

Nevermind. Stephen caught the derp. Should be !.

[ducky64]

Superseded by #515