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

Zero-length submodule name breaks things #1209

Closed
tpwrules opened this issue Mar 16, 2024 · 4 comments · Fixed by #1376 · May be fixed by #1235
Closed

Zero-length submodule name breaks things #1209

tpwrules opened this issue Mar 16, 2024 · 4 comments · Fixed by #1376 · May be fixed by #1235
Labels
Milestone

Comments

@tpwrules
Copy link
Contributor

The following design:

from amaranth import *
from amaranth.back import verilog

class Bar(Elaboratable):
    def elaborate(self, platform):
        m = Module()
        m.d.sync += Signal().eq(1)
        return m

class Foo(Elaboratable):
    def elaborate(self, platform):
        m = Module()
        m.submodules[""] = Bar()
        return m

print(verilog.convert(Foo(), ports=[]))

gives the following exception:

Traceback (most recent call last):
  File "~/test.py", line 16, in <module>
    print(verilog.convert(Foo(), ports=[]))
  File "~/venv/lib/python3.10/site-packages/amaranth/back/verilog.py", line 61, in convert
    verilog_text, name_map = convert_fragment(fragment, ports, name, emit_src=emit_src, strip_internal_attrs=strip_internal_attrs, **kwargs)
  File "~/venv/lib/python3.10/site-packages/amaranth/back/verilog.py", line 39, in convert_fragment
    rtlil_text, name_map = rtlil.convert_fragment(*args, **kwargs)
  File "~/venv/lib/python3.10/site-packages/amaranth/back/rtlil.py", line 1029, in convert_fragment
    empty_checker=empty_checker).emit()
  File "~/venv/lib/python3.10/site-packages/amaranth/back/rtlil.py", line 308, in emit
    self.emit_submodules()
  File "~/venv/lib/python3.10/site-packages/amaranth/back/rtlil.py", line 516, in emit_submodules
    self.builder.cell(f"\\{dotted_name}", submodule.name[-1], ports={
  File "~/venv/lib/python3.10/site-packages/amaranth/back/rtlil.py", line 166, in cell
    name = self._make_name(name, local=False)
  File "~/venv/lib/python3.10/site-packages/amaranth/back/rtlil.py", line 67, in _make_name
    elif not local and name[0] not in "\\$":
IndexError: string index out of range

I suggest that m.submodules[""] = ... have the same behavior as m.submodules += .... While it would be silly to directly write the former, it might arise from programmatic name generation, and should either work properly or immediately throw an error.

@whitequark whitequark added the bug label Mar 16, 2024
@whitequark whitequark added this to the 0.5 milestone Mar 22, 2024
@whitequark whitequark mentioned this issue Mar 22, 2024
77 tasks
@tpwrules
Copy link
Contributor Author

Upon further consideration it's probably best to reject "" as an invalid name rather than try to do something magic with it.

@whitequark
Copy link
Member

whitequark commented Mar 24, 2024

I suggest that m.submodules[""] = ... have the same behavior as m.submodules += .... While it would be silly to directly write the former, it might arise from programmatic name generation, and should either work properly or immediately throw an error.

I was actually thinking that this proposal seems OK. In fact, if you do m.submodules += they become "unnamed" and this is already surfaced in several places in the UI (like in error messages).

@tpwrules
Copy link
Contributor Author

The specific situation I had in mind was that if you do m.submodules[x] = foo, then bar = m.submodules[x], this cannot work if x == "".

I'm fine with the concept of unnamed modules, but if you accidentally make x == "", then having the above break might be confusing. It does seem unlikely though.

@whitequark
Copy link
Member

That's a good point, I forgot m.submodules[x] (reading) is a thing.

whitequark added a commit to whitequark/amaranth that referenced this issue Jun 10, 2024
This is semantically ambiguous and breaks the RTLIL emitter.

Fixes amaranth-lang#1209.
github-merge-queue bot pushed a commit that referenced this issue Jun 10, 2024
This is semantically ambiguous and breaks the RTLIL emitter.

Fixes #1209.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 participants