Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

For HasId.setRef, have first set win (with force override) #1655

Merged
merged 1 commit into from
Nov 5, 2020

Conversation

jackkoenig
Copy link
Contributor

@jackkoenig jackkoenig commented Nov 5, 2020

This is a refinement of the assertion added in #1616 then removed in
#1654. Because Records now set the refs of children upon binding,
later, unbound Records could incorrectly override the refs. The first
set should win.

This relates to firesim/firesim#653.

This issue was found from the following declaration (relevant code pasted below):
https://github.com/firesim/firesim/blob/c2d8e3a46e59222e115a1fdaa7267592e1d3c503/sim/firesim-lib/src/main/scala/bridges/SerialBridge.scala#L39

Basically, we have a Record that intentionally does not clone its argument (not even upon calling cloneType on itself). This means that all clones of this Record have the same exact Data object as a field.

// We're using a Record here because reflection in Bundle prematurely initializes our lazy vals
class HostPortIO[+T <: Data](protected val targetPortProto: T) extends TokenizedRecord {
  val fromHost = new HostReadyValid
  val toHost = Flipped(new HostReadyValid)
  val hBits  = targetPortProto

  val elements = collection.immutable.ListMap(Seq("fromHost" -> fromHost, "toHost" -> toHost, "hBits" -> hBits):_*)

  override def cloneType: this.type = new HostPortIO(targetPortProto).asInstanceOf[this.type]
  ...
}
object HostPort {
  def apply[T <: Data](gen: T): HostPortIO[T] = new HostPortIO(gen)
}
...
val hPort = IO(HostPort(new SerialBridgeTargetIO)) // SerialBridgeTargetIO is a Bundle, doesn't really matter
 ...
val targetReset = tFire & hPort.hBits.reset

This is totally an API-hole that should be closed, but it is the existing API and is used intentionally in FireSim.

The problem is that there are 2 instances of HostPortIO, the first one which is unbound (call it "A") and the second which is bound as the IO with name hPort (call it "B"). Note that they each have the same SerialBridgeTargetIO object as their element named hBits.

In Chisel v3.4.0 and before, during onModuleClose, "A" sets the ref of the SerialBridgeTargetIO to point to itself, then "B" overrides that and sets the ref to point to itself (which is the correct result). This is due to the order in which they were constructed (thus being "correct by coincidence").

In #1616, we changed the behavior so that Records set the refs of their children upon binding, and only if never bound do they set it during onModuleClose. This means that now "B" sets the ref first, then "A" overrides it. This PR fixes this issue by ensuring the first set wins (unless overridden which is for BlackBox only and is described in #1616).

If this sounds sketchy, it is, but fixing this API hole should occur on a major release. This fix is intended only to preserve existing behavior on the 3.4.x release line.

Contributor Checklist

  • [NA] Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • [NA] Did you add appropriate documentation in docs/src?
  • Did you state the API impact?
  • Did you specify the code generation impact?
  • Did you request a desired merge strategy?
  • [NA] Did you add text to be included in the Release Notes for this change?

No release notes because bug is not-yet released

Type of Improvement

  • bug fix

API Impact

No impact

Backend Code Generation Impact

No impact

Desired Merge Strategy

  • Squash

Release Notes

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (3.2.x, 3.3.x, 3.4.0, 3.5.0) ?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you mark as Please Merge?

@jackkoenig jackkoenig added the Bugfix Fixes a bug, will be included in release notes label Nov 5, 2020
@jackkoenig jackkoenig added this to the 3.4.x milestone Nov 5, 2020
@jackkoenig jackkoenig requested a review from a team as a code owner November 5, 2020 22:08
@jackkoenig jackkoenig added Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. and removed Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. labels Nov 5, 2020
@jackkoenig
Copy link
Contributor Author

#1654 didn't backport apparently so need to get that in first before backporting this

This is a refinement of the assertion added in #1616 then removed in
 #1654. Because Records now set the refs of children upon binding,
later, unbound Records could incorrectly override the refs. The first
set should win.
@jackkoenig jackkoenig added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Nov 5, 2020
@mergify mergify bot merged commit 8187318 into master Nov 5, 2020
mergify bot pushed a commit that referenced this pull request Nov 5, 2020
This is a refinement of the assertion added in #1616 then removed in
 #1654. Because Records now set the refs of children upon binding,
later, unbound Records could incorrectly override the refs. The first
set should win.

(cherry picked from commit 8187318)
@mergify mergify bot added the Backported This PR has been backported label Nov 5, 2020
mergify bot added a commit that referenced this pull request Nov 6, 2020
…1657)

This is a refinement of the assertion added in #1616 then removed in
 #1654. Because Records now set the refs of children upon binding,
later, unbound Records could incorrectly override the refs. The first
set should win.

(cherry picked from commit 8187318)

Co-authored-by: Jack Koenig <koenig@sifive.com>
@jackkoenig jackkoenig deleted the set-ref-first-wins branch December 1, 2020 19:47
jackkoenig pushed a commit that referenced this pull request Feb 28, 2023
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported This PR has been backported Bugfix Fixes a bug, will be included in release notes Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants