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

Fix use of read-only refs on rhs of connect in compatibility mode #854

Merged
merged 1 commit into from
Jul 11, 2018

Conversation

jackkoenig
Copy link
Contributor

#820 changed the implementations of ref and lref to do error checking which was problematic for compatibility mode. I changed legacyConnect to manually use the old implementation of Node(this) which fixes the issue in this instance. I'm a bit worried that other examples of this can creep up so we'll have to be vigilant but I have searched for all uses of ref and lref and I can't see any other problematic cases.

Type of change: bug fix

Impact: no functional change

Development Phase: implementation

Release Notes
Fix bug preventing the use of the output of aggregate Muxes on the rhs of a connection in compatibility mode.

@jackkoenig jackkoenig requested review from ducky64 and ucbjrl July 9, 2018 18:44
Copy link
Contributor

@ucbjrl ucbjrl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see checking moved out of pushCommand() and its arguments. Ideally, errors would be diagnosed (and potentially either recovered from or subverted) before getting down to pushCommand().

@jackkoenig
Copy link
Contributor Author

Are you referring to the implementations of lref and ref? eg.

  // Internal API: returns a ref that can be assigned to, if consistent with the binding
  private[chisel3] def lref: Node = {
    requireIsHardware(this)
    topBindingOpt match {
      case Some(binding: ReadOnlyBinding) => throwException(s"internal error: attempted to generate LHS ref to ReadOnlyBinding $binding")
      case Some(binding: TopBinding) => Node(this)
      case opt => throwException(s"internal error: unknown binding $opt in generating LHS ref")
    }
  }

That was changed by #820 and I think changing it again potentially beyond the scope of this PR. That being said, those appear to be additional internal checks (not the primary checks) and the issues that could cause them from a user perspective are already caught (eg. see all of the requireIsHardwares on Bits operations and https://github.com/freechipsproject/chisel3/blob/9591d690f5a3d71fd5de7a0fb2d67a63e8079f48/chiselFrontend/src/main/scala/chisel3/core/Data.scala#L296)

@ucbjrl
Copy link
Contributor

ucbjrl commented Jul 9, 2018

Ok - for both scope and error issues.

I defer to @ducky64 for approval.

Copy link
Contributor

@ducky64 ducky64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this works, this appears fine (mostly because this is limited to legacyConnect). That being said, this could potentially create odd behavior, and this pattern was what led to something like true.B.asClock erroring out in FIRRTL.

@jackkoenig
Copy link
Contributor Author

From compatibility mode's perspective, several things error out in FIRRTL so it's fine 😃

@jackkoenig jackkoenig merged commit 61810bc into master Jul 11, 2018
@ucbjrl ucbjrl added this to the 3.2.0 milestone Jul 17, 2018
@edwardcwang edwardcwang deleted the fix-compat-agg-mux branch March 15, 2019 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants