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

Fix Windows build by forcing initialization order, fixes #4068 #4101

Merged
merged 1 commit into from
Jan 6, 2024

Conversation

mmicko
Copy link
Member

@mmicko mmicko commented Jan 2, 2024

Windows build is crashing on start, since registering of passes crashed. IdString can not be used as variable default values since order of initialization is not guaranteed, and MinGW compiler always doing it in different order than Clang.
Using static is not strictly necessary but makes sure to do this only once per run.
Have tried to make this with less changes as possible, but guess m_ prefix for variables should be removed since they are local.
FYI doing init as part of constructor initialization does not help, since that is actually same moment of execution as previous code.

@mmicko mmicko requested review from povik and nakengelhardt January 2, 2024 10:34
@povik
Copy link
Member

povik commented Jan 2, 2024

I suppose another approach is to paste all these IDs into kernel/constids.inc but given how vendor-specific those IDs are, I don't know if we want to do that.

@povik
Copy link
Member

povik commented Jan 2, 2024

Btw if it was e.g.

const RTLIL::IdString split_cell_type("$__QLF_TDP36K");

instead of

const RTLIL::IdString split_cell_type = ID($__QLF_TDP36K);

that would still be an issue, right?

@mmicko
Copy link
Member Author

mmicko commented Jan 2, 2024

@povik Right, using kernel/constids.inc should not be option for vendor specific things, and second example would fail as well since IdString global is not yet constructed. We had similar issue in past, but it was always affecting just Windows :( and we were not able to reproduce these issues on Linux.

@QuantamHD
Copy link
Contributor

Would this change have potentially resolved it? #4091

@cr1901
Copy link
Contributor

cr1901 commented Jan 5, 2024

Just stopping by to confirm that this fixes my issue as described in #4068. Tyvm @mmicko for the patch :).

@povik povik merged commit 1ddb089 into master Jan 6, 2024
34 checks passed
@mmicko mmicko deleted the micko/fix_init_order branch May 9, 2024 07:51
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.

5 participants