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

AArch64: Fix C++ mangling of va_list & apply ltsmaster's IndirectByvalRewrite fix #2813

Merged
merged 6 commits into from
Aug 17, 2018

Conversation

kinke
Copy link
Member

@kinke kinke commented Aug 11, 2018

C++ va_list is mangled as std::__va_list struct. According to clang IR, it's special in the sense of not ever being passed by value.

Let's do the same and pass a pointer while at the same time mangling the function as taking the arg by value.

@kinke
Copy link
Member Author

kinke commented Aug 11, 2018

There's an ABI fix wrt. ExplicitByvalRewrite in ltsmaster which isn't in master yet...

@kinke
Copy link
Member Author

kinke commented Aug 11, 2018

But a va_copy limitation which isn't in master... argh.

@kinke kinke changed the title AArch64: Fix C++ mangling of va_list & partial va_arg support [WIP] AArch64: Fix C++ mangling of va_list & partial va_arg support Aug 11, 2018
@joakim-noah
Copy link
Contributor

This pull gets ldc master to be able to bootstrap itself on Android/AArch64, fixing the link error I mentioned in #2811 yesterday. You may want to add the stdlib/dmd tests and that self-bootstrap to Shippable in this pull to make sure it doesn't regress.

hfaToArray.applyTo(arg, arg.ltype);
} else {
if (isReturnVal) {
integerRewrite.applyTo(arg);
Copy link
Member Author

@kinke kinke Aug 13, 2018

Choose a reason for hiding this comment

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

ltsmaster doesn't use the IntegerRewrite at all; it uses CompositeToArray64 for rewriting returned structs (those not returned via sret). That casts the struct's address to [N x i64]* and loads from it, so if the struct is smaller than 8 bytes (or its size not a multiple of 8), this might be an issue (for args too, not just the return value). IntegerRewrite only works for power-of-2 sizes IIRC, so that's probably not really robust either. Edit: Nope, it takes care of that.

@kinke kinke changed the title [WIP] AArch64: Fix C++ mangling of va_list & partial va_arg support [WIP] AArch64: Fix C++ mangling of va_list & apply ltsmaster's IndirectByvalRewrite fix Aug 13, 2018
@kinke
Copy link
Member Author

kinke commented Aug 13, 2018

The IndirectByvalRewrite fix resolves #2150.

@kinke
Copy link
Member Author

kinke commented Aug 14, 2018

Yay, now really fully buildable with latest ltsmaster. And less than 9 minutes for building the unittest runners is pretty impressive.

@kinke
Copy link
Member Author

kinke commented Aug 14, 2018

The tests hang at some point, independent from ltsmaster/self host compiler, and apparently during the Phobos unittests as well as during dmd-testsuite.

@joakim-noah
Copy link
Contributor

I can confirm that ldc master still bootstraps itself and passes the same tests on linux and Android/AArch64 with this pull. The segfault in the tests for core.sys.linux.stdio that I mentioned in #2153 yesterday also goes away with David's fix that you pulled in now.

@kinke kinke changed the title [WIP] AArch64: Fix C++ mangling of va_list & apply ltsmaster's IndirectByvalRewrite fix AArch64: Fix C++ mangling of va_list & apply ltsmaster's IndirectByvalRewrite fix Aug 14, 2018
@kinke
Copy link
Member Author

kinke commented Aug 14, 2018

Not sure what to make of the

warning: Error disabling address space randomization: Operation not permitted
During startup program terminated with signal SIGSEGV, Segmentation fault.
No stack.

output for various DMD tests (apparently not gdb related as the first Google results would suggest).

@joakim-noah
Copy link
Contributor

Probably related to the unoptimized codegen issues I mentioned in #2153: I just tried it and can reproduce many segfaults and hangs with dmd-testsuite-debug on Kai's linux/AArch64 VPS. I suggest disabling building the debug versions until we figure that out.

@joakim-noah
Copy link
Contributor

Looking good, once I get that druntime commit in, pretty much all the druntime tests should pass too.

When do you plan to tag the 1.11 release and start building the compiler binaries? I'd like to get your suggested method of initializing TLS data on Android in for 1.11, ie replacing the delimiter symbols with parsing the ELF headers, but I'll have to get the path of the ELF file differently for shared libraries, which is how Android GUI apps (apks) are distributed, versus command-line binaries, which is easier and seems to be working in my experiments so far.

@kinke
Copy link
Member Author

kinke commented Aug 15, 2018

When do you plan to tag the 1.11 release and start building the compiler binaries?

This weekend (basically just waiting for this to be merged). 1.11 has been ready from my side for weeks, and now additionally got somewhat improved for AArch64 at the very last moment. I guess we'll have a 1.12 beta in a few weeks, so if you don't manage with the Android TLS stuff in those few days, that's fine too IMO.

@joakim-noah
Copy link
Contributor

No, a couple days is plenty of time, should be able to get that in.

C++ va_list is mangled as `std::__va_list` struct. According to clang
IR, it's special in the sense of not ever being passed by value.

Let's do the same and pass a pointer while at the same time mangling the
function as taking the arg by value.
Some of those make the whole test run hang.
@kinke kinke merged commit 9bd4fb2 into ldc-developers:master Aug 17, 2018
@kinke kinke deleted the shippable branch August 17, 2018 16:13
@joakim-noah
Copy link
Contributor

Looking into the Android TLS setup, but thought of a different way to do it than just calling functions from rt.backtrace.elf, which will take some time to investigate. Feel free to do the 1.11 release whenever you like, no need to wait on me.

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.

None yet

2 participants