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

Don't predeclare a struct as a class #114

Merged

Conversation

mortbopet
Copy link
Contributor

@mortbopet mortbopet commented Sep 6, 2024

Holy moly this took a long time to debug.

MSVC gives absolutely zero warnings about if a struct/class is predeclared as one or the other, during compilation.

However, linking also needs to happen. And as it turns out, whether a thing is a class or struct is part of the MSVC name mangling spec. Hence, we'd end up with e.g. this function (which references SymbolInfo) as being defined in it's object file as the following symbol:

lookupSymbolsByLocation@SymbolTable@textplan@substrait@io@@QEBA?AV?$vector@PEBUSymbolInfo@textplan@substrait@io@@V?$allocator@PEBUSymbolInfo@textplan@substrait@io@@@std@@@std@@AEBVLocation@234@@Z

However, things which reference SymbolInfo, but which may have pulled in the incorrect predeclaration prior, will reference it as an incorrect symbol:

lookupSymbolsByLocation@SymbolTable@textplan@substrait@io@@QEBA?AV?$vector@PEBVSymbolInfo@textplan@substrait@io@@V?$allocator@PEBVSymbolInfo@textplan@substrait@io@@@std@@@std@@AEBVLocation@234@@Z ...

e.g. notice the difference between PEBUSymbolInfo and PEBVSymbolInfo. Enqueue strange linking issues galore.

@mortbopet mortbopet changed the title Don't predeclare a class as a struct Don't predeclare a struct as a class Sep 6, 2024
@mortbopet
Copy link
Contributor Author

@EpsilonPrime unsure why CI fails here; this shouldn't change anything wrt. the underlying tests.

@EpsilonPrime
Copy link
Member

Yeah, really weird as I wouldn't expect changes to file locations to affect the correctness of one test. I will check into this when I get in.

@EpsilonPrime
Copy link
Member

The issue is that the timezones "US/Central" and "UTC" are no longer being found in the timezone database. I am able to reproduce this issue on iOS as well. Still looking for what changed between today and yesterday.

@EpsilonPrime
Copy link
Member

Looks like the timezone database was updated yesterday:

https://lists.iana.org/hyperkitty/list/tz-announce@iana.org/thread/IZ7AO6WRE3W3TWBL5IR6PMQUL433BQIE/

Copy link
Member

@EpsilonPrime EpsilonPrime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one's totally my fault. I'm surprised it wasn't caught by all of the lint and build systems we have. Thanks for spending the time to find this!

@EpsilonPrime
Copy link
Member

(The timezone issue appears to be fixed by #120 . Will merge this after that PR lands.)

@mortbopet mortbopet force-pushed the dev/mpetersen/struct_is_not_class branch from 59b8a2c to 9163892 Compare September 7, 2024 07:50
@EpsilonPrime EpsilonPrime merged commit 3ea39df into substrait-io:main Sep 7, 2024
3 checks passed
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