-
Notifications
You must be signed in to change notification settings - Fork 603
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
Add lexical scope checks to Assert, Assume and Printf #2706
Add lexical scope checks to Assert, Assume and Printf #2706
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great idea but the implementation needs to be more about checking the lexical scope of the individual Data within a Printable
rather than of the Printable
objects themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks great, I have some style suggestions but the functionality is all there and looks great!
Move checkScope to object Printable and update callsites
Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>
c6902d7
to
1525a1e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
(cherry picked from commit f462c9f)
When passing a
Printable
toassert
,assume
orprintf
, the scope of where thePrintable
was constructed should be the same as where it is being referenced. This is currently not checked, which makes it possible to construct cases where invalid FIRRTL is generated. Specifically, if thePrintable
is constructed in an inner module and referenced from an outer module, the resulting FIRRTL can have references to objects like wires or temporaries that are local to the inner module.For example, the following snippet
outputs the problematic FIRRTL
printf
where the wire
w
internal toInnerPrintable
is being accessed outside of its scope.This PR adds checks to
assert
,assume
andprintf
to ensure that the Builder scopes atPrintable
construction and reference are the same.Contributor Checklist
docs/src
?Type of Improvement
API Impact
N/A
Backend Code Generation Impact
Printfs, asserts and assumes will not be present in the generated Verilog if the Printable scopes are invalid.
Desired Merge Strategy
Release Notes
Printables (and other objects?) used in printf, assert and assume can now only be referenced in the scopes in which they were constructed
Reviewer Checklist (only modified by reviewer)
3.4.x
, [small] API extension:3.5.x
, API modification or big change:3.6.0
)?Enable auto-merge (squash)
, clean up the commit message, and label withPlease Merge
.Create a merge commit
.