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

dmd.expression: Use align(8) for alignment of UnionExp #8907

Merged
merged 1 commit into from
Nov 4, 2018

Conversation

ibuclaw
Copy link
Member

@ibuclaw ibuclaw commented Nov 3, 2018

SPARC port is killed with a sigbus error when building any module. This was due to an unaligned load in IntegerExp::toInteger from inside interpret() where UnionExp is prevalent.

The reason is because real_t may itself be a struct that is not suitably aligned for accessing dinteger_t (8-bytes).

Just waiting to hear back that this is fine, otherwise the corrective action would be to add a dinteger_t field so that UnionExp is suitably aligned for both native real_t and dinteger_t types.

It would be nice if dmc++ supported __attribute__(aligned(8))) instead of having this here...

@ibuclaw ibuclaw added GDC Gnu D Compiler Trivial typos, formatting, comments C++ Port labels Nov 3, 2018
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @ibuclaw!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8907"

@thewilsonator
Copy link
Contributor

Wont real be overkill in terms of alignment then if dinteger_t will do?

@WalterBright
Copy link
Member

DMC++ supports pragma pack(8);

https://www.digitalmars.com/ctg/pragmas.html#pack

@ibuclaw
Copy link
Member Author

ibuclaw commented Nov 4, 2018

DMC++ supports pragma pack(8);

Heh... Let's do that then. :-)

@ibuclaw
Copy link
Member Author

ibuclaw commented Nov 4, 2018

Warning, C++ headers will become ugly.

@thewilsonator
Copy link
Contributor

@ibuclaw this is labelled c++ Port but is targeted to master, so which is it? ;)

@ibuclaw ibuclaw removed the C++ Port label Nov 4, 2018
@ibuclaw
Copy link
Member Author

ibuclaw commented Nov 4, 2018

Wont real be overkill in terms of alignment then if dinteger_t will do?

UnionExp has both IntegerExp and RealExp, potentially either of the toInteger() or toReal() functions could use an instruction that has strict alignment requirements if dealing with native types. The highest common denominator should be used to avoid any problems with one or the other.

@ibuclaw this is labelled c++ Port but is targeted to master, so which is it? ;)

Whoops, C++ port will follow, as this is affecting gcc trunk. :-)

@ibuclaw ibuclaw changed the title dmd.expression: Use long double field for alignment of UnionExp dmd.expression: Use align(8) for alignment of UnionExp Nov 4, 2018
@ibuclaw
Copy link
Member Author

ibuclaw commented Nov 4, 2018

Updated to use align(8) and whatever is the equivalent for the host C++ compiler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge GDC Gnu D Compiler Trivial typos, formatting, comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants