firrtl icon indicating copy to clipboard operation
firrtl copied to clipboard

Add --dump-on-crash to StageOptions and Shell

Open timsnyder opened this issue 4 years ago • 4 comments

Contributor Checklist

  • [ ] Did you add Scaladoc to every public function/method?
  • [ ] Did you update the FIRRTL spec to include every new feature/behavior?
  • [ ] Did you add at least one test demonstrating the PR?
  • [x] Did you delete any extraneous printlns/debugging code?
  • [x] Did you specify the type of improvement?
  • [x] Did you state the API impact?
  • [x] Did you specify the code generation impact?
  • [x] Did you request a desired merge strategy?
  • [ ] Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • code refactoring Moves the code for the annotationSeq trace logger into an AnnotationSeq.serialize method
  • code cleanup I did a little code gardening to fix a typo in a comment and update a couple of outdated scaladoc blocks. Not sure how you guys feel about having that stuff mixed in.
  • new feature/API Adds --dump-on-crash see below.

API Impact

Adds a new --dump-on-crash to StageOptions that writes the raw compiler annotations to error.anno.json when an exception is caught in CatchExceptions

Backend Code Generation Impact

None known.

Desired Merge Strategy

squashing is fine.

Release Notes

Future Work

  • split the FirrtlCircuitAnnotation out into a .fir file
  • add support for encapsulating a CircuitState with a caught exception. For example, one could catch in firrtl.Transform#remapAnnotations, augmenting the exception with the 'before' and 'after' CircuitState. Then, CatchExceptions can unpack the additional debug info and rethrow original exception.

Reviewer Checklist (only modified by reviewer)

  • [ ] Did you add the appropriate labels?
  • [ ] Did you mark the proper milestone (1.2.x, 1.3.0, 1.4.0) ?
  • [ ] Did you review?
  • [ ] Did you check whether all relevant Contributor checkboxes have been checked?
  • [ ] Did you mark as Please Merge?

timsnyder avatar Oct 21 '20 19:10 timsnyder

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Oct 21 '20 19:10 CLAassistant

I'm not sure why Travis isn't running...

jackkoenig avatar Oct 21 '20 21:10 jackkoenig

Does this need a test? If so, can you suggest where I try to add one?

timsnyder avatar Oct 21 '20 21:10 timsnyder

A test could be good to lock in that this does the right thing.

I'd look at how https://github.com/freechipsproject/firrtl/blob/master/src/test/scala/firrtlTests/stage/FirrtlMainSpec.scala works. You could either piggy-back off of FirrtlMainTest and write one of those, checking any expected prints and files, or use the lower-level catchWrites API that you can use to trap if a file write occurs and then verify what the filename is.

seldridge avatar Oct 21 '20 23:10 seldridge