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

ifndef vanishes from includes #49

Closed
flaviens opened this issue Oct 14, 2022 · 3 comments
Closed

ifndef vanishes from includes #49

flaviens opened this issue Oct 14, 2022 · 3 comments

Comments

@flaviens
Copy link

flaviens commented Oct 14, 2022

Hi there!

When Morty pickles a header file such as

`ifndef BP_COMMON_CACHE_ENGINE_SVH
`define BP_COMMON_CACHE_ENGINE_SVH

typedef logic newtype_t;

`endif

then it becomes

`define BP_COMMON_CACHE_ENGINE_SVH

typedef logic newtype_t;

and this may cause tools to crash if the header is included multiple times.
I created an example here.

What workarounds would you suggest? Or is there anything that can be done in Morty to fix that problem?

I'm quite surprised to not have stumbled into this problem before so maybe I'm doing something wrong here.
Maybe doing such a typedef in a svh is also not legal, but can be found in the wild such as here.

Thanks!
Flavien

@micprog
Copy link
Member

micprog commented Oct 17, 2022

This is because morty parses files individually, doing macro resolution of each, not taking into account the previously parsed files and the defines contained within. I attempted to make use of some of the parser's features to collect all the defines from the parsed files, but I haven't properly tested or investigated any further issues. You can give the following branch a shot if you like: https://github.com/pulp-platform/morty/tree/collect_defines
Let me know if this works, or if you encounter any issues with this change.

@flaviens
Copy link
Author

Again, that was fast 😍 ! That should fix it, however it's difficult to estimate whether it would break backward compatibility.
You're the best placed to decide!
Thanks!

@micprog
Copy link
Member

micprog commented Oct 18, 2022

I will likely modify this feature to be enabled/disabled with a flag before merging to ensure compatibility isn't broken.

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

No branches or pull requests

2 participants