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

Disable doFormat under GNU compiler. #56

Merged
merged 4 commits into from
Jun 15, 2023
Merged

Conversation

schveiguy
Copy link
Member

@schveiguy schveiguy commented Jun 8, 2023

Remove unittest and compilation for GNU compiler. Instead of obscure error from druntime internals, this provides a static assert.

Should fix #55 and obsolete #54

Note: the problem with doFormat is that it uses a specific form of va_arg that GDC does not support. A future edit to this library can re-enable if someone figures out how to use the templated form:

va_list argptr;
int arg;
arg = va_arg!int(argptr); // OK
va_arg(argptr, typeid(int), &arg); // Not OK

But I think we can merge this now, and if we have people looking for support, they can submit a further PR to re-enable with the correct changes. However, eschewing all undead support from GDC for this one module is not worth it.

For reference, here is the function that is disabled for GDC: https://github.com/dlang/dmd/blob/f2b3b97eb327a1d409decd2510950d91ec35fb52/druntime/src/core/vararg.d#L22

@schveiguy
Copy link
Member Author

ping @ibuclaw is this sufficient?

@schveiguy
Copy link
Member Author

Hm... probably should add gdc to the github tests? Does that work?

@anon17
Copy link

anon17 commented Jun 9, 2023

It's a better message, but compilation will still fail, because undead.stream instantiates doFormat unconditionally. It happens when dub package is compiled: it's compiled whole.

@anon17
Copy link

anon17 commented Jun 9, 2023

version(GNU)
void doFormat(scope void delegate(dchar) putc, TypeInfo[] arguments, va_list ap)
{
	assert(false, "GNU D compiler does not support doFormat");
}
else
void doFormat()(scope void delegate(dchar) putc, TypeInfo[] arguments, va_list ap)
{
...
}

@schveiguy
Copy link
Member Author

Thanks, I'll search for any others.

@schveiguy
Copy link
Member Author

Looks like that's the only other one.

@schveiguy
Copy link
Member Author

Regarding the suggestion to just use a runtime assert, we could do that also, but I don't think that's the right way to handle this. You should know if you use doformat, it's going to fail at compile time.

@schveiguy
Copy link
Member Author

schveiguy commented Jun 9, 2023

I had to do the runtime check in undead.stream since it's not a template. Now should compile with GNU. I also added gdc to the compiler list, let's see if it actually works...

Answer: nope... Does anyone know the right way to do this?

@quickfur
Copy link
Member

quickfur commented Jun 9, 2023

Unrecognized compiler gdc? Is there some config file somewhere that defines this?

@@ -1206,10 +1209,17 @@ class Stream : InputStream, OutputStream {

// writes data with optional trailing newline
OutputStream writefx(TypeInfo[] arguments, va_list argptr, int newline=false) {
Copy link
Member

Choose a reason for hiding this comment

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

Will the fact that this function takes a va_list cause any problems with GDC?

Copy link
Member Author

@schveiguy schveiguy Jun 9, 2023

Choose a reason for hiding this comment

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

No, it's not va_list, it's the way the list is used. You can still get the args using compile-time parameters. e.g. va_arg!int(argptr)

I should have put this in the description, but this is the issue: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108763

Copy link
Member Author

@schveiguy schveiguy Jun 9, 2023

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I added more context in the description

@schveiguy
Copy link
Member Author

See the link to the setup-dlang repository, @quickfur

@schveiguy schveiguy force-pushed the patch-1 branch 5 times, most recently from dcc43cc to b00d738 Compare June 9, 2023 20:58
@quickfur
Copy link
Member

quickfur commented Jun 9, 2023

Tests are passing so far... let's hope this goes through.

@schveiguy
Copy link
Member Author

Tests are passing so far.

hehe, it was still running dmd instead of gdc! I hate the way YAML works here...

Hopefully I got it right this time.

@schveiguy schveiguy force-pushed the patch-1 branch 3 times, most recently from 9980838 to d9a6eaa Compare June 9, 2023 21:24
@schveiguy
Copy link
Member Author

woohoo! gdc passing

@ibuclaw
Copy link
Member

ibuclaw commented Jun 10, 2023

But I think we can merge this now, and if we have people looking for support, they can submit a further PR to re-enable with the correct changes. However, eschewing all undead support from GDC for this one module is not worth it.

For reference, here is the function that is disabled for GDC: https://github.com/dlang/dmd/blob/f2b3b97eb327a1d409decd2510950d91ec35fb52/druntime/src/core/vararg.d#L22

👍

In case anyone reads this far down. The rationale behind not supporting is because vararg passing is something done at the discretion of the compiler - eg shortcuts are taken to optimise specific cases sometimes (ie: fitting a struct/array into one register, rather than on the stack).

This makes codegen for varargs consistent with itself at compile-time, but not guaranteed to be compatible with a thirdly party runtime implementation (or another compiler).

This is especially true for D's Typeinfo based vararg implementation, which lacks lots of information around how va arguments are classified (there's no support in D for retrieving AVX types over RTTI varargs IIRC). This is even worse for non-x86 targets - especially Arm64 - so it's easier to say "don't support that" instead of having a half broken implementation.

@schveiguy
Copy link
Member Author

Why is MacOS taking days to run? Is it still trying to run jobs that were on old force-pushes? I must have pushed this branch like 15 times trying to get the CI to work...

@schveiguy
Copy link
Member Author

Why is MacOS taking days to run?

Answer from kinke in slack:

GHA dropped macOS 10.* images a while ago, only 11+ available nowadays.

Updated to 11, see if this works...

@schveiguy
Copy link
Member Author

OK, all green!

@WalterBright
Copy link
Member

D's Typeinfo based vararg implementation, which lacks lots of information around how va arguments are classified (there's no support in D for retrieving AVX types over RTTI varargs IIRC).

In D's defense, that feature was designed and implemented before AVX types existed. Is there a bugzilla issue for this?

@WalterBright
Copy link
Member

The same problem should also exist in DMD and LDC. doFormat() should also work correctly for 32 bit code.

@schveiguy
Copy link
Member Author

I just want to make sure this is understood, before we talk about the correctness of GDC's or other compiler's choices. People want to use undead with gdc (maybe undead.xml), but cannot because you can't build the library with gdc, since doFormat fails to build.

This relieves that problem.

If we want to fix doFormat, or gdc, or how all the compilers handle these types of varargs, I don't really mind, but it makes zero sense that nobody using gdc can use the entirety of undead for this one problem.

@WalterBright
Copy link
Member

@schveiguy I agree. But what gdc supports or does not is up to @ibuclaw

@ibuclaw
Copy link
Member

ibuclaw commented Jun 15, 2023

I just want to make sure this is understood, before we talk about the correctness of GDC's or other compiler's choices. People want to use undead with gdc (maybe undead.xml), but cannot because you can't build the library with gdc, since doFormat fails to build.

Inderd, so why is this still open?

@ibuclaw ibuclaw merged commit 627f4b8 into dlang:master Jun 15, 2023
@schveiguy schveiguy deleted the patch-1 branch June 16, 2023 00:53
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.

doformat should be completely disabled for GDC
5 participants