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

Work around Migen's finalization being subtly order dependent #1

Merged
merged 1 commit into from
Nov 13, 2018

Conversation

whitequark
Copy link
Contributor

Right now, in Migen, if the module tree looks like this:

top
\---- probe
\---- microscope

and finalizing microscope makes the module tree look like this:

top
\---- probe ----- ps_arm
\---- microscope

whether any Verilog will actually be generated for ps_arm is
dependent on traversal order of submodules, which cannot be
relied upon. This causes microscope /dev/ttyX singles to hang.

Avoid this by making finalization of probes order-independent.

@sbourdeauducq Not only this broke microscope in a nasty way, but this is one of the worst footguns in Migen that I've hit constantly in Glasgow... I think something should be done about it.

@sbourdeauducq
Copy link
Member

It's a migen bug, if a finalize function creates a submodule, then Verilog should always be emitted for that submodule.

@whitequark
Copy link
Contributor Author

Are you opposed to merging this in meantime? I feel like it makes the logic in microscope more straightforward, too.

@sbourdeauducq
Copy link
Member

What about this in Insert:

def do_finalize(self):
    if self.registry.is_enabled(self):
        self.create_insert_logic()

Right now, in Migen, if the module tree looks like this:

    top
    \---- probe
    \---- microscope

and finalizing `microscope` makes the module tree look like this:

    top
    \---- probe ----- ps_arm
    \---- microscope

whether any Verilog will actually be generated for `ps_arm` is
dependent on traversal order of submodules, which cannot be
relied upon. This causes `microscope /dev/ttyX singles` to hang.

Avoid this by making finalization of probes order-independent.
@whitequark
Copy link
Contributor Author

Done

@sbourdeauducq sbourdeauducq merged commit 02cffc3 into master Nov 13, 2018
@whitequark whitequark deleted the order branch November 23, 2019 04:50
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.

2 participants