Skip to content

Translate-c on non i386 drops stdcall annotation #4287

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

Open
JesseRMeyer opened this issue Jan 25, 2020 · 14 comments
Open

Translate-c on non i386 drops stdcall annotation #4287

JesseRMeyer opened this issue Jan 25, 2020 · 14 comments
Labels
translate-c C to Zig source translation feature (@cImport) upstream An issue with a third party project that Zig uses.
Milestone

Comments

@JesseRMeyer
Copy link

Consider WNDPROC from WinUser.h:

typedef LRESULT (CALLBACK* WNDPROC)(HWND, UINT, WPARAM, LPARAM);

Where CALLBACK is defined as (context included):

#elif (_MSC_VER >= 800) || defined(_STDCALL_SUPPORTED)
#define CALLBACK    __stdcall

Yet translate-c generates:

pub const WNDPROC = ?fn (HWND, UINT, WPARAM, LPARAM) callconv(.C) LRESULT;

with a .C, not .Stdcall convention.

For those not in the know, the difference between the two calling conventions is whether or not the caller or callee is responsible for clearing the stack after the call concludes. Getting this mixed up can make for 'fun' debugging sessions.

@JesseRMeyer
Copy link
Author

In this case, translate-c is dropping the calling convention.

Consider RegisterClassA, again from WinUser.h (formatting included, may be a hint):

WINUSERAPI
ATOM
WINAPI
RegisterClassA(
    _In_ CONST WNDCLASSA *lpWndClass);

where WINAPI is defined as, in fact, just under CALLBACK:

#define WINAPI      __stdcall

yet translate-c generates:

pub extern fn RegisterClassA(lpWndClass: [*c]const WNDCLASSA) ATOM;

@LemonBoy
Copy link
Contributor

yet translate-c generates

Hey, let's not jump to (wrong) conclusions! It's clang that doesn't give a damn about your __stdcall modifier because you didn't specify the right target. Try again with -target i386-windows ;)

@JesseRMeyer
Copy link
Author

JesseRMeyer commented Jan 25, 2020

Thanks -- I'll give that a shot.

Although I'll bite back a little on the principle attitude you show. I shouldn't have to know all of rotational kinematics to learn how to turn a wrench. In other words, I shouldn't have had to specify that target, since that is the host target. The null hypothesis is to specify the target if that is not the target I'm, well, targeting. So, fair enough. It's not translate-c's fault directly in this instance, but it is in another sense, by not sheltering me from poor defaults.

@andrewrk
Copy link
Member

The default target is the native target. Your native target is probably x86_64. The C code in question has a preprocessor macro to define stdcall to be empty string for your target. So there's nothing Zig could do - the C code says the calling convention is ccc.

The closest idea that has any possibility to address this is #3028. Applying it to translate-c is an interesting idea, but pretty advanced, and quite possibly so complicated as to not be worthwhile.

@LemonBoy
Copy link
Contributor

The C code in question has a preprocessor macro to define stdcall to be empty string for your target. So there's nothing Zig could do - the C code says the calling convention is ccc.

Nah, it's just that Clang helpfully ignores that attribute on targets that don't support it but it also shows warning: '__stdcall' calling convention is not supported for this target [-Wignored-attributes].
Since the code that translates the clang diagnostic messages into Zig ones ignores everything that's not an error the user is completely oblivious to that.

This said yeah, the default target is the one of the system Zig runs on.

@JesseRMeyer
Copy link
Author

JesseRMeyer commented Jan 25, 2020

I submitted this as LemonBoy posted his reply above ... may be irrelevant.

The C code in question has a preprocessor macro to define stdcall to be empty string for your target.

Good observation. __stdcall isn't defined. It's more similar to a cl.exe intrinsic than a C keyword so I understand the dilemma. What I don't grok is the difficulty in teaching translate-c about '__stdcall'. While that may open up a question of how many 'non' c features translate-c supports, from a practical point of view, __stdcall offers a very high pay-off, (relatively low cost investment?) especially for win32 devs. And not including it leaves a sour taste in the mouth, speaking from experience. At the least, I would hope that an error message be included for this, otherwise you'll see a stream of win32 devs complaining about it.

@JesseRMeyer
Copy link
Author

@LemonBoy Interesting. So is there anyway to convince Clang that __stdcall should be delivered through to translate-c?

@LemonBoy
Copy link
Contributor

So is there anyway to convince Clang that __stdcall should be delivered through to translate-c?

Use -target i386-windows? That's also how you convince the real clang not to ignore it.

@JesseRMeyer
Copy link
Author

@LemonBoy I apologize if this is obvious for you, and I appreciate your keeping with me. I thought what Andrew said, and that you confirmed, was that Zig was telling Clang the correct target already.

So am I correct that Zig is only specifying the hardware target to Clang, and not the operating system target? I'm feel like I'm about to start kicking a dead horse, but shouldn't Zig be telling Clang this on my behalf already?

The real reason I asked you again is because specifying that target is causing a bit of a wild goose chase finding all the various include directories that Windows.h refers to, which I did not need to before. Unsure if that's because Clang was shielding me from that or if it's something else.

@JesseRMeyer
Copy link
Author

JesseRMeyer commented Jan 25, 2020

But hey, thanks @LemonBoy after chasing all those paths down, __stdcall is now respected, at least after some spot checking. Don't know if I've set a trap for myself going to i386 rather than x86_64 though. Seems this is more a Clang issue than a translate-c issue now. Clang should have worked correctly with x86_64-windows. Until educated otherwise, I'll file this as a workaround.

@andrewrk
Copy link
Member

andrewrk commented Jan 25, 2020

Ah my mistake about the preprocessor macro. That's still something that C code can and does do in practice. But not in this instance. It's unfortunate that clang ignores it at the parser level. It would be better if we could access the data regardless of the target. Maybe the clang devs would be amenable to a patch.

@JesseRMeyer what target would you expect zig to pass to clang? What target do you believe is currently being passed?

@JesseRMeyer
Copy link
Author

@andrewrk I'm assuming zig.exe is passing the reported (native) targets, which together form x86_64-windows. Why overriding that sensible (and true) default with i386 (essentially lying to clang that the target is 32bit -- right?) works is bizarre to me. But @LemonBoy thinks that is the right target. It's at least right in the sense that clang cooperates, and I'd like some education on whether or not is it somehow generally right when that's not the target of the host.

@JesseRMeyer
Copy link
Author

I'll also state that translate-c has been a huge boon to my win32 productivity with zig. It's a very nice tool!

@andrewrk
Copy link
Member

andrewrk commented Jan 25, 2020

Your assumption about what zig is doing is correct. It’s not correct to pass i386 as the target (unless you are intentionally cross compiling for that target). That merely demonstrates clangs alternative behavior so you can understand why this issue exists.

@andrewrk andrewrk added this to the 1.1.0 milestone Jan 25, 2020
@andrewrk andrewrk changed the title Translate-C does not always preserve callconv. Translate-c on non i386 drops stdcall annotation Jan 25, 2020
@andrewrk andrewrk added translate-c C to Zig source translation feature (@cImport) upstream An issue with a third party project that Zig uses. labels Jan 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
translate-c C to Zig source translation feature (@cImport) upstream An issue with a third party project that Zig uses.
Projects
None yet
Development

No branches or pull requests

3 participants