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

Use self hosted translate-c for cImport #3973

Closed
wants to merge 11 commits into from

Conversation

Vexu
Copy link
Member

@Vexu Vexu commented Dec 22, 2019

Closes #1964

@andrewrk
Copy link
Member

oh my god this is so beautiful

@Vexu Vexu force-pushed the stage-2-cimport branch 2 times, most recently from b022863 to 913f7cc Compare December 23, 2019 07:47
@@ -21,9 +21,9 @@ pub fn addCases(cases: *tests.TranslateCContext) void {
, &[_][]const u8{
\\pub export fn foo() void {
\\ var a: c_int = undefined;
\\ var b: u8 = @as(u8, 123);
\\ var b: u8 = @intCast(u8, 123);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't we prefer @as?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but there seem to be some places @intCast is required and I'm not sure they can be done separately.

@FireFox317 FireFox317 mentioned this pull request Dec 27, 2019
@frmdstryr
Copy link
Contributor

frmdstryr commented Dec 27, 2019

I added the macro changes from #3984 in 17319ba, it'd be nice to merge those into this.

@Vexu

This comment has been minimized.

@daurnimator

This comment has been minimized.

@daurnimator

This comment has been minimized.

@Vexu Vexu force-pushed the stage-2-cimport branch 2 times, most recently from b1a7d04 to a175454 Compare December 29, 2019 11:27
@daurnimator

This comment has been minimized.

@daurnimator

This comment has been minimized.

@daurnimator
Copy link
Contributor

zig translate-c fail.zig on an empty file fail.zig results in a crash. (it doesn't make sense to pass a .zig file to translate-c, but I wasn't expecting a crash!)

@Vexu

This comment has been minimized.

@daurnimator

This comment has been minimized.

@Vexu

This comment has been minimized.

Vexu and others added 2 commits December 29, 2019 19:23
- fix use of undefined value
- fix parenexprclass result not being suppressed
- add an error and a TODO for access of an anonymous field
@frmdstryr
Copy link
Contributor

Thanks for merging.

If you're looking for more source to test with I've been trying to convert the stm32h7 headers.

One other issue is things like #define __IO volatile gets translated as pub const __IO = @"volatile"; when it should be replacing the types where it's used. I haven't looked at how to fix this yet..

Also it'd be nice to be able to retain comments and the order since currently converting makes them practically unreadable.

@Vexu
Copy link
Member Author

Vexu commented Dec 29, 2019

One other issue is things like #define __IO volatile gets translated as pub const __IO = @"volatile"; when it should be replacing the types where it's used. I haven't looked at how to fix this yet..

Not sure what you mean here. If you want to use __IO as a replacement for the volatile keyword then that's not possible in Zig.

Also it'd be nice to be able to retain comments

Might be possible but low priority.

and the order

Order of what?

@frmdstryr
Copy link
Contributor

Not sure what you mean here. If you want to use __IO as a replacement for the volatile keyword then that's not possible in Zig.

Oh sorry, I don't want to use __IO in place of volatile in zig. I want to be able to translate-c code which does this (ex) and have the keywords properly translated.

Order of what?

Order of which macros, functions, etc are defined. For example stm32h743xx.h currently translates to stm32h743xx.zig which seems to be in a completely random order (this was done with translate-c not translate-c-2 but they both do this). This is also a low priority.

@andrewrk
Copy link
Member

Order of which macros, functions, etc are defined. For example stm32h743xx.h currently translates to stm32h743xx.zig which seems to be in a completely random order (this was done with translate-c not translate-c-2 but they both do this). This is also a low priority.

Yeah this is planned, but not a requirement to replace translate-c with the self-hosted version.

@andrewrk
Copy link
Member

One other issue is things like #define __IO volatile gets translated as pub const __IO = @"volatile"; when it should be replacing the types where it's used. I haven't looked at how to fix this yet..

The translation of the macro is correct. What you're looking for is support for translating volatile things, which is a different topic than macros. The feature you're looking for is independent of whether someone uses __IO or directly uses the volatile keyword.

@daurnimator
Copy link
Contributor

Errors don't seem to get reported well: if the clang invocation doesn't work, then we only report "C import failed". To get the actual error I have to use --verbose-cc and then run the clang command line myself.

@andrewrk
Copy link
Member

The best way to troubleshoot with the self hosted version is to use --verbose-cimport and then open the translated zig code file in a text editor. The errors will be comments inline with the translated code.

@daurnimator
Copy link
Contributor

The best way to troubleshoot with the self hosted version is to use --verbose-cimport and then open the translated zig code file in a text editor. The errors will be comments inline with the translated code.

That of course is only available if the cimport succeeded (which is not the case in the scenario above).

@andrewrk
Copy link
Member

Oh I see, C compilation errors. That is a regression that I would like to rectify before merging this.

@frmdstryr
Copy link
Contributor

I added support for #4006 to the tokenizer in https://github.com/Vexu/zig/compare/stage-2-cimport...frmdstryr:translate-c-macro-stringify-concat?expand=1

Not sure what's the best way to integrate it into translate-c.

@andrewrk
Copy link
Member

I started working on merging this in the branch Vexu-stage-2-cimport.

@andrewrk
Copy link
Member

andrewrk commented Jan 1, 2020

Progress: the Vexu-stage-2-cimport branch is now passing all the tests for macOS as well as Linux. Next up: Windows.

@Vexu
Copy link
Member Author

Vexu commented Jan 1, 2020

Re: 42945a2
Parameter names were being mangled unconditionally for a reason.

int foo(int bar) {
    bar = 2;
}
int bar = 4;

no longer produces valid zig as bar is being redeclared

pub export fn foo(arg_bar: c_int) c_int {
    var bar = arg_bar;
    bar = 2;
}
pub export var bar: c_int = 4;

@andrewrk
Copy link
Member

andrewrk commented Jan 1, 2020

Ah, thanks for pointing that out. I think we're going to need 2 passes over the clang AST unit. Once to determine all the names in the global namespace, and then a second one to actually do the translation. I'm on it.

@andrewrk
Copy link
Member

andrewrk commented Jan 1, 2020

I opened the branch as a pull request in #4025, let's continue efforts there. (pull requests can be filed against branches other than master)

@andrewrk andrewrk closed this Jan 1, 2020
@Vexu Vexu deleted the stage-2-cimport branch January 2, 2020 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

make translate-c self-hosted
4 participants