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

add ImportC C compiler #12507

Merged
merged 1 commit into from
May 18, 2021
Merged

add ImportC C compiler #12507

merged 1 commit into from
May 18, 2021

Conversation

WalterBright
Copy link
Member

This adds an ANSI C11 C compiler to dmd so it can import and compile C files directly. See changelog/ImportC.md for details.

This is a prototype implementation, known shortcomings are in the changelog.

It doesn't alter D in any way.

@WalterBright WalterBright added the Review:Trivial typos, formatting, comments label May 9, 2021
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

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 run digger -- build "master + dmd#12507"

@UplinkCoder
Copy link
Member

This is a 3000 line pr... and preliminary support for parsing a related but different language it's labeled trivial...

@WalterBright
Copy link
Member Author

Undefined symbols for architecture x86_64:
  "Token::toChars(unsigned short)", referenced from:
      test_tokens() in cxxfrontend.o
  "Target::isVectorOpSupported(Type*, unsigned short, Type*)", referenced from:
      test_target() in cxxfrontend.o

This is a bit baffling. The declarations in the code are correct.

src/dmd/cparse.d Outdated Show resolved Hide resolved
@WalterBright WalterBright force-pushed the ImportC branch 2 times, most recently from 6c98d04 to b9d030b Compare May 9, 2021 21:39
@maxhaton
Copy link
Member

maxhaton commented May 9, 2021

C:

typedef float real;

typedef struct impl {
    void* x;
    char lazy;
} module;

void does(int x)
{
    module copy;
}

D:

import cdecl;

extern(C)
int main()
{
    impl g;

    module x;
    pragma(msg, __traits(allMembers, impl)); //tuple("x", "lazy")
    return g.lazy;
}

@adamdruppe
Copy link
Contributor

This appears completely useless to me. First, it doesn't "just work" since you need to run cpp over the file before you can use it, and since that expands macros, it must be done for each different target, meaning it must be part of the build system. This offers no advantage over the (not great) existing solutions and shares many of their problems including creating huge files.

Even if that just worked, C bindings tend to rely on the preprocessor to do common and necessary tasks like defining constants. Without that, you can't even reasonably use real world C headers. Possibly a limited processor could translate them, but even that data is gone after cpp goes through it, so there's no real hope to recover it into a usable form.

I think trying to run the cpp over a whole D file like dpp does is problematic... but like something must be done. dstep's approach actually does a reasonably good job, at the cost of requiring a bit of hand editing and some setting up of other files too.

I do think there's some potential in integrating with dmd... but this PR as it is looks like more harm than good. And I'm not sure it is worth the time sink it will take to get it to a positive point.

@WalterBright
Copy link
Member Author

@adamdruppe All good points, and I anticipated this reaction. But may I ask that the discussion of "should we do this at all" be moved to the newsgroup? I expect it to be long, and threading in the n.g. will make it easier to navigate. I already started the thread on it.

Please keep this one confined to a review of the PR. Thanks!

@Imperatorn
Copy link
Contributor

I think this is an interesting prototype. I don't see it as useless. It needs some work ofc, but it could potentially become quite handy

@maxhaton
Copy link
Member

maxhaton commented May 9, 2021

struct context;

struct context {
    float fps;
}; 

Fails to compile with Error: no struct-declarator-list for struct context, which in turn breaks every C header I've tried so far.

changelog/ImportC.md Outdated Show resolved Hide resolved
@WalterBright
Copy link
Member Author

@maxhaton That one fails because the semantic processing is in the D part. I haven't addressed that yet.

@12345swordy
Copy link
Contributor

@WalterBright try to follow the "separations of concern" design principle when implementing the c compiler. Don't be afraid of creating new files for this.

@WalterBright
Copy link
Member Author

Buildkite Style appears to have a problem:

/var/lib/buildkite-agent/builds/ci-agent-6af57101-f5f2-406d-935a-c6100716b586-8/dlang/dmd/dmd/src/dmd/cparse.d(219:18)[error]: Expected `identifier` instead of `typedef`

Looks like Style's list of tokens needs updating.

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented May 16, 2021

I don't know why the bot doesn't add the comments but you can always refer to their website, see e.g.
https://app.codecov.io/gh/dlang/dmd/compare/12507/tree/src/dmd/cparse.d

@WalterBright
Copy link
Member Author

Thank you, that's what I need!

@WalterBright WalterBright force-pushed the ImportC branch 2 times, most recently from b3cd9aa to 90da945 Compare May 16, 2021 03:22
@WalterBright
Copy link
Member Author

@MoonlightSentinel unfortunately the coverage results for cparse.d are not being updated.

@MoonlightSentinel
Copy link
Contributor

All coverage pipelines sucessfully uploaded their report for cparse.d, so any problems should be on CodeCov's side.

@WalterBright
Copy link
Member Author

At the top the coverage page says:

Unmerged Base Commits
The base reference contains commits not present in the head reference.
Please merge the base reference into the head reference to see the most
up to date coverage information.

which sounds relevant, but I don't know specifically what it means.

@WalterBright
Copy link
Member Author

Probably the most practical way to move forward with coverage testing is to pull this.

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented May 16, 2021

CodeCov recently changed the algorithm which determines the base commit (changelog), seems like it selects the wrong base commit for this repository.....

Probably the most practical way to move forward with coverage testing is to pull this.

You can run the coverage test locally:

rdmd src/build.d clean
rdmd src/build.d ENABLE_COVERAGE=1
rdmd test/run.d compilable/testcstuff1.d

At first glance, a fail_compilation test for the various error's is missing.

@WalterBright
Copy link
Member Author

I'm going to defer the fail_compilation tests at this time, mainly because they are less important, and due to the fluid nature of this prototype are subject to constant change.

@WalterBright
Copy link
Member Author

You can run the coverage test locally

Thanks, that's working beautifully!

@WalterBright
Copy link
Member Author

It's about as far as coverage testing will go without modifying the rest of the front end.

Comment on lines +1064 to +1065
scope p = new CParser!AST(this, buf, cast(bool) docfile,
cast(ubyte) target.c.longsize, cast(ubyte) target.c.long_doublesize, cast(ubyte) target.c.twchar_t.size());
Copy link
Member

Choose a reason for hiding this comment

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

Could you instead pass a const copy/reference to target.c, this would allow me to quickly add the other types immediately after this gets pulled.

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 wanted cparse.d to have minimal dependencies on anything else.

Copy link
Member Author

Choose a reason for hiding this comment

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

What you could do is move TargetC to globals.d, just the data fields, not the member functions. That would sever its dependencies. Change twchar_t to a size rather than a Type, then the struct can stand alone.

Copy link
Member

Choose a reason for hiding this comment

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

CParser is already a template though, so the cparse.d module need not import dmd.target.

@dlang-bot dlang-bot merged commit 40541dd into dlang:master May 18, 2021
@MoonlightSentinel
Copy link
Contributor

So open discussions and remaining issues like #12507 (comment) were ignored again...

src/dmd/cparse.d Show resolved Hide resolved
src/dmd/cparse.d Show resolved Hide resolved
@ibuclaw
Copy link
Member

ibuclaw commented Dec 10, 2022

This PR introduced a regression https://issues.dlang.org/show_bug.cgi?id=23548

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ImportC Pertaining to ImportC support Merge:auto-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.