8000 VHPI: Preserve original name case for CaseNameP and FullCaseNameP · Issue #723 · nickg/nvc · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

VHPI: Preserve original name case for CaseNameP and FullCaseNameP #723

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.

Sign up for GitHub

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

Jump to bottom
Open
Forty-Bot opened this issue Jun 18, 2023 · 14 comments
Open

VHPI: Preserve original name case for CaseNameP and FullCaseNameP #723

Forty-Bot opened this issue Jun 18, 2023 · 14 comments
Labels
vhpi

Comments

@Forty-Bot
Copy link
Contributor

CaseNameP and FullCaseNameP are specified to use "...the case of letters in the identifier of the ... declaration." However, we currently always return these properties as uppercase.

I don't think this is particularly high priority (and would likely take a lot of effort to resolve). However, the cocotb folks requested that a tracking issue be created for this deviation.
@nickg
Copy link
Owner
nickg commented Jun 24, 2023

I don't really want to track the original case of identifiers in addition to the normalised uppercase identifier so I've implemented this in a slightly hacky/best-effort way for declarations using the source location to find the original identifier in the file and fall back to the upper-case name if that doesn't work. Seems to work ok in my limited testing.

nickg added a commit that referenced this issue Jun 24, 2023
@nickg
Partial implementation of vhpiCaseNameP for declarations. Issue #723
93285e9
@nickg nickg added the vhpi label Jun 24, 2023
@Forty-Bot
Copy link
Contributor Author

There were two main bugs I saw related to CaseName:

  • Cocotb asks for something in lowercase and is surprised when we give it something in uppercase
  • Cocotb gets a handle via vhpi_scan and gets the same handle later via vhpi_handle_by_name and gets confused when the names aren't the same.

So this really needs to be accompanied by making vhpi_handle_by_name case-sensitive, which also means converting the other (Full)CaseNames (in c_name etc.).

That said I've been pretty happy with my solution of just converting all requested identifiers to uppercase in cocotb.

@nickg
Copy link
Owner
nickg commented Jun 24, 2023

That said I've been pretty happy with my solution of just converting all requested identifiers to uppercase in cocotb.

Yes if they can live with that it would be much simpler.

@marlonjames marlonjames mentioned this issue Jun 26, 2024
Open
@ktbarrett
Copy link
Contributor

Yes if they can live with that it would be much simpler.

Unfortunately we can't since we service SV code as well. We can't know what PLI interface returned an object, nor the original source language of any object, due to the abstraction of the GPI. Nor do we want to know, as that's the whole point of abstracting the interface away.

@ktbarrett
Copy link
Contributor
ktbarrett commented Feb 4, 2025 •
edited
Loading

Just to make a suggestion, you don't need to keep both the name and the normalized uppercase version around, just keep the original name and hash the uppercase version for use in lookups.

@axman6
Copy link
axman6 commented Feb 4, 2025

I'm currently running into this too, we have cocotb simulations which work in GHDL when using lower case names (which definitely look more at home in python land). Given VHDL isn't case sensitive, it seems reasonable to be able to refer to anything in the hierarchy in a case insensitive way.

We can't change out tests just to use uppercase as it would break our already working GHDL tests (and I'm running into other issues and crashes with nvc which mean we can't swap it out at the moment).

(Thanks @ktbarrett for your help elsewhere).

@nickg
Copy link
Owner
nickg commented Feb 4, 2025 •
edited
Loading

Given VHDL isn't case sensitive, it seems reasonable to be able to refer to anything in the hierarchy in a case insensitive way.

I agree, but that's not what's being asked for here. The cocotb developers want the CaseNameP property implemented properly so that it returns the original spelling of the identifier. That would make the lookups in Python case sensitive. It's annoying because it's literally the only thing in VHDL that requires preserving this information. I suspect the IEEE just standardised one vendor's existing implementation which happened to do that rather than deliberately designing it this way.

Just to make a suggestion, you don't need to keep both the name and the normalized uppercase version around, just keep the original name and hash the uppercase version for use in lookups.

Sure, but a nice property of canonicalising the names and interning the strings is that you can do O(1) comparisons just by comparing pointers, whereas even if you make names that differ in case hash to the same value you still need to do a full comparison to handle collisions. I suspect this is what I'll end up doing though.

@ktbarrett
Copy link
Contributor

I suspect the IEEE just standardised one vendor's existing implementation which happened to do that rather than deliberately designing it this way.

It's what I would do. That property needs to return names in a consistent casing so it doesn't levy case insensitive handling on the rest of the user code. Of the options available, preserving the original casing gives more information and doesn't require rewriting strings that may never be looked up.

In my mind it's best to think about the case insensitivity as a feature of lookup and not fundamental to any of the objects. Its just syntax sugar.

nickg added a commit that referenced this issue Feb 16, 2025
< 8000 div class="AvatarStack flex-self-start " >
@nickg
Add experimental --preserve-case analysis option. Issue #723
ee98692
@nickg
Copy link
Owner
nickg commented Feb 16, 2025

I've added an experimental --preserve-case analysis option that does this. I tried it with cocotb and all the tests passed except test_discovery (which needs a special case for NVC removed).

@ktbarrett
Copy link
Contributor

Thanks! I'll give it a try soon.

@ktbarrett
Copy link
Contributor
ktbarrett commented Mar 5, 2025 •
edited
Loading

Just rebased and tested cocotb/cocotb#3899 with the option and it's still failing without the uppercase nonsense. Does the --preserve-case not apply to packages for some reason?

@nickg
Copy link
Owner
nickg commented Mar 5, 2025

You missed the second place where it analyses files:

diff --git a/src/cocotb_tools/makefiles/simulators/Makefile.nvc b/src/cocotb_tools/makefiles/simulators/Makefile.nvc
index 5c9151d5c2ee..60f3a4f72eef 100644
--- a/src/cocotb_tools/makefiles/simulators/Makefile.nvc
+++ b/src/cocotb_tools/makefiles/simulators/Makefile.nvc
@@ -38,7 +38,7 @@ analyse: $(VHDL_SOURCES) $(SIM_BUILD) $(CUSTOM_COMPILE_DEPS)
 
        $(foreach LIB_VAR,$(VHDL_LIB_ORDER), \
                $(CMD) $(EXTRA_ARGS) --work=$(LIB_VAR):$(SIM_BUILD)/$(LIB_VAR) -L $(SIM_BUILD) -a $(VHDL_SOURCES_$(LIB_VAR)) --preserve-case $(COMPILE_ARGS) && ) \
-       $(CMD) $(EXTRA_ARGS) --work=$(RTL_LIBRARY):$(SIM_BUILD)/$(RTL_LIBRARY) -L $(SIM_BUILD) -a $(VHDL_SOURCES) $(COMPILE_ARGS)
+       $(CMD) $(EXTRA_ARGS) --work=$(RTL_LIBRARY):$(SIM_BUILD)/$(RTL_LIBRARY) -L $(SIM_BUILD) -a $(VHDL_SOURCES) --preserve-case $(COMPILE_ARGS)
 
 $(COCOTB_RESULTS_FILE): analyse $(CUSTOM_SIM_DEPS)
        $(RM) $(COCOTB_RESULTS_FILE)

Then it fails with this error:

AssertionError: assert False
 +  where False = isinstance(LogicArrayObject(cocotb_package_pkg_1::eight_logic), LogicObject)
 +    where LogicArrayObject(cocotb_package_pkg_1::eight_logic) = HierarchyObject(cocotb_package_pkg_1).eight_logic

@ktbarrett
Copy link
Contributor
ktbarrett commented Mar 5, 2025 •
edited
Loading

😞 Me big dumb. It works now. Thanks again! This issue can probably be closed.

One more question is: why is this an commandline option and not just standard? Needing to pass options to get standards compliant behavior seems like something a Big 3 would do. Or is it like a feature flag to see if it will break stuff, but to be turned on by default at some point?

@nickg
Copy link
Owner
nickg commented Mar 6, 2025

I'll make it the default at some point but I'd prefer it to get some more testing while it's still opt-in. I also need to fix up a bunch of error message checks in my tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
vhpi
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@nickg @axman6 @Forty-Bot @ktbarrett
0