Dependencyapi phase4 #256

sequencer · 2021-01-05T07:24:13Z

Fixes for reviews and CIs

ducky64

I'm getting this odd feeling of deja vu that some of these comments I'm making here we've already discussed prior during calls or in previous iterations of this PRs. Just because it's a different PR doesn't mean the issues don't still need to be addressed.

I have significant issues with how you've defined SimulatorInterface, details in the line-by-line comments. This really seems like the kind of thing that should be discussed beforehand, probably even at the concept stage before you've started writing lots of code around it - because otherwise (and as we're seeing now) you might have a lot of code to re-write.

ducky64 · 2021-01-13T01:47:44Z

src/main/scala/chiseltest/ChiselScalatestTester.scala

+      case se: StageError =>
+        se.getCause match {
+          case exception: FailedExpectException =>
+            val scalatestException = new TestFailedException(


TODO check exception printout on console

It has been tested in the chiseltest.tests.BasicTest

ducky64 · 2021-01-13T01:50:23Z

src/main/scala/chiseltest/ChiselScalatestTester.scala

+            val scalatestException = new TestFailedException(
+              exception,
+              exception.stackTraceElements.indexWhere(ste =>
+                ste.getClassName == "chiseltest.package$testableData" && ste.getMethodName == "expect"


Don't you already do the stack trace pruning in phase 2? Why is it done again here? Can you also define in the class TestFailedException where the stack trace should begin so it's part of the interface definition?

Alternatively, TestFailedException doesn't really need the particular StackTraceElement, if the stack trace associated with the exception is properly pruned?

Don't you already do the stack trace pruning in phase 2? Why is it done again here?
This was a bug fix for scalatest, I didn't run scalatest on phase 2.

Can you also define in the class TestFailedException where the stack trace should begin so it's part of the interface definition?

I think we can define it in a implicit def under ChiselScalatestTester, if we do that, TestFailedException will explicitly depend on ScalaTest.

Don't you already do the stack trace pruning in phase 2? Why is it done again here?

I git blame this. it was bug fixes. In phase 2, I didn't test the code, I fix those bugs introduced by this refactor on phase 4, so it might contain related changes.

Can you also define in the class TestFailedException where the stack trace should begin so it's part of the interface definition?

fixed.

Alternatively, TestFailedException doesn't really need the particular StackTraceElement, if the stack trace associated with the exception is properly pruned?

Fixed.

ducky64 · 2021-01-13T01:52:43Z

src/main/scala/chiseltest/backends/SimulatorBackend.scala

 package chiseltest.backends

 import firrtl.AnnotationSeq
 import logger._

 import scala.sys.process.ProcessLogger

+/** [[SimulatorBackend]] is used to generate a binary executable.
+  * It should will compile FIRRTL to a [[SimulatorInterfaceAnnotation]].


"should will"?

src/main/scala/chiseltest/backends/SimulatorBackend.scala

src/main/scala/chiseltest/backends/SimulatorInterface.scala

ducky64 · 2021-01-13T02:07:38Z

src/main/scala/chiseltest/internal/ThreadedBackend.scala

-        expectStackDepth != 0,
-        s"Failed to find expect in stack trace:\r\n${trace.getStackTrace.mkString("\r\n")}"
+      val fullStackTraces = (trace.getStackTrace ++ getParentTraceElements).filterNot(ste =>
+        ste.getClassName.startsWith("chiseltest.internal.")


This significantly changes the behavior - this is filtering out, instead of filtering all before or after. The criteria is also looser. Both are subtle, but makes the code more brittle.

Yes, I know that under the old one, changes elsewhere means this also needs to be changed too. But we have tests to catch that, and I'd prefer code errs on the stricter and safer side.

BasicTest has correspond test on this.
maybe you can take a review on running chiseltest.tests.BasicTest:

it should "fail on expect mismatch" in { assertThrows[exceptions.TestFailedException] { test(new StaticModule(42.U)) { c => c.out.expect(0.U) } } }

And FaultLocatorTest work well under this refactor.

ducky64 · 2021-01-13T02:09:43Z

src/main/scala/chiseltest/internal/ThreadedBackend.scala

+    simulatorInterface.peek(dataNames(clk)) match {
+      case Some(x) if x == BigInt(1) => true
+      case _                         => false
+    }


As mentioned in a previous phase, these should fail noisily. Similarly below.

I'll add an additional API to enable/disable this.
In the future, it's possible to peek a non-exist(constant-propagated) target inside Module.

Can you give me a use case where you would want to peek a non-existent target and where some arbitrary result would be useful?

Like, I can see the utility of maybe peekOption -> Option[BigInt], and punt to the upper layer to deal with what happens on a None / nonexistent wire, but I don't think (and I feel strongly about this, unless you have solid arguments the other way) anything should ever create results out of thin air, because this kind of non-intuitive data (even if expedient now) can lead unsuspecting users down a fruitless debugging rabbit hole.

And even with peekOption, I'm not sure what would be guidelines for when to use it vs. when not to, and I'd prefer for APIs not to be introduced until we have a reasonable use case for them.

I also feel strongly against a stateful API to toggle this kind of behavior.

For example: a API design here:
https://github.com/sequencer/diplomatictester/blob/4284f6919426186cc4d3e752bb0578d3104ce3cd/src/Driver.scala#L99
But corrupt is eliminated during constant propagation.
And if it directly fails:
https://github.com/sequencer/diplomatictester/blob/4284f6919426186cc4d3e752bb0578d3104ce3cd/tests/src/dutIOSpec.scala#L46
won't be able to run.

The key issue of this is: If we are peeking/poking a constant-propagated target, what behavior should be?
Or we can dontTouch those signal explicitly, and throw it like you said.

I would say the behavior should be to crash. Sure, a zero might work in the use case you have now, but might not work in another use case - so it's a bad idea to hardcode values created out of nothing. With a crash, at least it's noisy and the user gets a stack trace indicating what failed, and can add the needed dontTouch annotations. Compared to creating values - the user might just see a failed test, with an assertion that might not trigger until much later (so nothing hinting at the true root cause), with RTL that is actually good, and might not even think about needing the debug the test infrastructure itself.

ducky64 · 2021-01-13T02:10:20Z

src/main/scala/chiseltest/package.scala

@@ -233,6 +232,9 @@ package object chiseltest {
    Context().doTimescope(() => contents)
  }

+  /** get current elapsedTime in nano second. */
+  def elapsedNanoSeconds: Long = Context().simulatorInterface.elapsedNanoSeconds()


I don't think this should be part of the general public API.

API removed

src/test/scala/chiseltest/experimental/tests/VerilatorCoverageTests.scala

ducky64 · 2021-01-13T02:13:16Z

src/test/scala/chiseltest/tests/FaultLocatorTest.scala

-    exc.failedCodeFileNameAndLineNumberString.get should startWith ("DecoupledDriver.scala:")
-    exc.getMessage should include regex ("""\(lines in FaultLocatorTest\.scala:[^\)]*43.*\)""")
+    exc.failedCodeFileNameAndLineNumberString.get should startWith("DecoupledDriver.scala:")
+    exc.getStackTrace.mkString("\n") should include regex ("""\(FaultLocatorTest\.scala:[^\)]*43.*\)""")


TODO see if this behaves properly in ScalaTest.

ScalaTest should behave good here, you can directly print out getStackTrace.

1. Add TestNameAnnotation to get rid of TargetDirAnnotation auto inserting. 2. catch error with pattern match StageError in scalatest. 3. move Context to ThreadedBackend file. 4. update FaultLocatorTest, get rid of old source info. 5. fix for ThreadSafetyLocator. 6. bug fix for 2.11, stacktrace has different depth.

1. don't remove old test file now. 2. use --t-name to override test folder. 3. fix hardcoded path in tests.

sequencer · 2021-01-18T11:44:25Z

I'm going to create a commit with all fixes about this review.
It should happen later today.

1. change FailedExpectException API, directly use setStackTrace and getStackTrace API from Scala. 2. SimulatorBackend now not extends from LazyLogging. 3. Add trait SubprocessSimulatorBackend to log process stdout and stderr. 4. remove time related API from SimulatorInterface, add it in RecordSimulatorTime trait. 5. SimulatorInterface poke/peek API change to throw SignalNotFoundException if not find a signal in the symbol table. 6. bug fix for not use TestCommandAnnotation. 7. remove stale peek related code. 8. re-add VerilatorCoverageTests. 9. fixes for some typos.

ducky64 · 2021-04-15T00:40:00Z

@sequencer what's going on with this refactor? Are things still a work-in-progress, or has this been abandoned? If the former, let me know how I can help speed things along so other changes aren't blocked; if the latter then 🤷 we should at least clarify and get some closure.

edwardcwang · 2021-04-15T00:46:04Z

Didn't he already reply to all the code review issues that came up? I see the commit above.

ducky64 · 2021-04-15T00:54:33Z

I think we discussed this during a meeting, and I think the resolution was to avoid incidental refactors so the diff doesn't bloat, and that not everything needed to be an annotation or serializable. I haven't seen any movement since, though.

Since this was an one-of meeting (instead of the regularly recurring chisel-dev), I don't know if there was a notes doc. A quick search didn't surface anything.

sequencer · 2021-04-15T01:27:54Z

In our last meeting, IIRC, these PRs are rejected since it's hard to review.
I was busy with other thing recently, I think since this week I can spare 1 or 2 hours per day to rebase and sort my commits into a series of meaningful PRs and we can discuss after next dev-meeting.

ducky64 · 2021-04-15T02:26:45Z

Yes, I recall these PRs were rejected because of feature creep too.

The original post was more regarding whether you were working on a refactor for dependencyapi that didn't have all the feature / 'cleanup' creep - because my understanding from that meeting was that you were going to do a minimal set of changes to migrate to dependencyapi.

Whether you accomplish that be rebasing these commits or starting from 'scratch' (perhaps copying over lines / files to another branch) is up to you, but please smaller, self-contained, single-feature PRs...

ekiwi · 2021-06-14T16:28:56Z

This PR has gotten stale, so I will close is for now. Feel free to re-open.

sequencer mentioned this pull request Jan 5, 2021

DependencyAPI Phase 4 #200

Closed

sequencer force-pushed the dependencyapi_phase4 branch 2 times, most recently from 7cf9914 to f7b8e41 Compare January 6, 2021 04:26

ducky64 suggested changes Jan 13, 2021

View reviewed changes

sequencer added 13 commits January 18, 2021 06:31

disable VerilatorCoverageTests, should be re-enabled after F#1961.

6529201

add SPDX License Identifier.

5f84113

add scaladoc to chiseltest.backend, clean up api.

8897a4c

remove BackendInterface

b42a054

add resolveResult for SimulatorInterface.

ed368c6

update for code review.

7b76c95

delete old test files.

82ddca6

add Documentation to ChiselTestOptions.

9e9b1c3

add Documentation to AnalysisCircuit, fix a typo.

66a7242

Refactor TestNameAnnotation

fe0be20

1. don't remove old test file now. 2. use --t-name to override test folder. 3. fix hardcoded path in tests.

code style change on chiseltest.stage.phases.Checks

4011e8d

code style change on chiseltest.stage.phases.AddDefaults

1c22b3d

sequencer force-pushed the dependencyapi_phase4 branch from f7b8e41 to 1c22b3d Compare January 18, 2021 06:37

sequencer force-pushed the dependencyapi_phase3 branch from 641536c to e49911e Compare January 18, 2021 06:41

ekiwi closed this Jun 14, 2021

ekiwi deleted the dependencyapi_phase4 branch September 22, 2023 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dependencyapi phase4 #256

Dependencyapi phase4 #256

sequencer commented Jan 5, 2021

ducky64 left a comment

ducky64 Jan 13, 2021

sequencer Jan 18, 2021

ducky64 Jan 13, 2021

sequencer Jan 18, 2021

sequencer Jan 18, 2021

ducky64 Jan 13, 2021

sequencer Jan 18, 2021

ducky64 Jan 13, 2021

sequencer Jan 18, 2021

ducky64 Jan 13, 2021

sequencer Jan 18, 2021

ducky64 Jan 18, 2021 •

edited

Loading

sequencer Jan 18, 2021

sequencer Jan 18, 2021

ducky64 Jan 18, 2021 •

edited

Loading

ducky64 Jan 13, 2021

sequencer Jan 18, 2021

ducky64 Jan 13, 2021

sequencer Jan 18, 2021

sequencer commented Jan 18, 2021

ducky64 commented Apr 15, 2021

edwardcwang commented Apr 15, 2021

ducky64 commented Apr 15, 2021 •

edited

Loading

sequencer commented Apr 15, 2021

ducky64 commented Apr 15, 2021

ekiwi commented Jun 14, 2021

Dependencyapi phase4 #256

Dependencyapi phase4 #256

Conversation

sequencer commented Jan 5, 2021

ducky64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ducky64 Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ducky64 Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sequencer commented Jan 18, 2021

ducky64 commented Apr 15, 2021

edwardcwang commented Apr 15, 2021

ducky64 commented Apr 15, 2021 • edited Loading

sequencer commented Apr 15, 2021

ducky64 commented Apr 15, 2021

ekiwi commented Jun 14, 2021

ducky64 Jan 18, 2021 •

edited

Loading

ducky64 Jan 18, 2021 •

edited

Loading

ducky64 commented Apr 15, 2021 •

edited

Loading