Set StageError cause in ChiselStage

[seldridge]

Change ChiselStage to set the cause of a StageError due to a ChiselException to be the ChiselException. This should fix issues where libraries that want to inspect the stack trace can do so. Previously, this would do stack trace trimming and drop a no-cause StageError.

Related issue: #1114 (comment)

Type of change: bug report

Impact: no functional change

Development Phase: implementation

[chick]

Looks ok. Should there be a test to illustrate?

[jackkoenig]

Looks good to me as well. One thing I was wondering, instead of printlning the trimmed stack trace ourselves, what would happen if we override some of the methods in Throwable? https://docs.oracle.com/javase/7/docs/api/java/lang/Throwable.html

Perhaps then we can use normal printStackTrace() for both and it will make it easier for custom Stages to compose with this

[ucbjrl]

Unfortunately, this by itself, isn't sufficient. Driver.execute catches StageError exceptions and returns an error:

    val annosx = try {
      phases.foldLeft(annos)( (a, p) => p.transform(a) )
    } catch {
      /* ChiselStage and FirrtlStage can throw StageError. Since Driver is not a StageMain, it cannot catch these. While
       * Driver is deprecated and removed in 3.2.1+, the Driver catches all errors.
       */
      case e: StageError => annos
    }

    view[ChiselExecutionResult](annosx)

which eventually gets converted into another zero information exception in chisel3.iotesters.setupTreadleBackend:

    chisel3.Driver.execute(optionsManager, dutGen) match {
      case ChiselExecutionSuccess(Some(circuit), _, Some(firrtlExecutionResult)) =>
        ...
      case _ =>
        throw new Exception("Problem with compilation")
    }

[jackkoenig]

@ucbjrl I think we should reconsider how we compose things and update those places to not catch Errors. As described in the Java documentation: "An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch."

[ucbjrl]

Perhaps it's time to revisit #361?

[seldridge]

Good catch, @ucbjrl.

I expect the Driver shouldn't be catching anything. In 3.1.5 it didn't... This fails some DriverSpec tests, though (checking what's going on).

Hot take: the sooner we can remove Driver, the sooner we can move to a unified exception model...

[seldridge]

Actually, @ucbjrl: I think what's happening is the correct behavior of stack trace trimming:

1. Elaboration fails and ChiselStage will print a verbose trimmed stack trace on stdout
2. A StageError gets thrown
3. The Driver converts this to a ChiselExecutionFailure
4. iotesters converts this to an information-less exception

Even though (4) happens, the information is still provided to the user. This is, as far as I can tell, how #931 was intended to work.

The contract of StageError is that the thrower is providing all information to the user to debug the problem (here, a trimmed stack trace on stdout). Hence, StageError may legally provide zero information on what is going on because the thrower has already provided that information. This is likely an anti-pattern.

@jackkoenig has suggested that stack trace trimming should instead directly manipulate the stack trace. This may be a better approach. Perhaps we should implement something like this instead?

[ucbjrl]

Right, the problem is the failure isn't available programmatically (without some contortions). Chisel is a library and may not always be the top application. One could argue that we shouldn't be printing errors without some negotiation with/advice from the enclosing application.

[jackkoenig]

Printing generally isn't great, imagine if there's bug in MIDAS Chisel code that isn't supposed to be exposed to the user, they can catch and report a better user error, but Chisel is still going to print the stack trace as if the Chisel code itself is user code.

I think there's general agreement about finding a more composable solution, the real question is if we should do this for 3.3 or perhaps do this for 3.4.

[seldridge]

Hot take 2️⃣: We're late enough on 3.3, let's just fix it. (I volunteer...)

[ucbjrl]

yes, lgtm