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

fix Issue 24026 - ImportC: ICE on nested C initializer 2 #15375

Merged
merged 1 commit into from Jul 5, 2023

Conversation

WalterBright
Copy link
Member

No description provided.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jul 3, 2023

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
24026 regression ImportC: ICE on nested C initializer 2

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 "stable + dmd#15375"

@dkorpel
Copy link
Contributor

dkorpel commented Jul 3, 2023

I tried compiling the C library that found this bug with this PR, and there's still errors:

typedef struct {
    struct {
        char data[24];
    };
    int length;
} ES;

ES empty = {{.data = {0}}, .length = 0};
Error: `[ constant-expression ]` expected for C array element initializer `{.short_data={0}}`

Do you want me to open a new bugzilla issue for that?

@dkorpel
Copy link
Contributor

dkorpel commented Jul 3, 2023

Another test case:

typedef struct {
    int k;
    struct {
        struct {
            struct {
                int s;
            } f;
        };
    };
} SH;

SH data = (SH) {
    .k = 0,
    {{.f = {.s = 0}}}
};
source/test.c(14): Error: unrecognized C initializer `{.f={.s=0}}`

@WalterBright
Copy link
Member Author

This baffles me. The failing test case works fine on my Ubuntu machine.

Also, the new test cases are all part of the same problem, so no need to add them to the bugzilla issue.

@dkorpel
Copy link
Contributor

dkorpel commented Jul 3, 2023

Also, the new test cases are all part of the same problem, so no need to add them to the bugzilla issue.

To clarify, I got those error messages when compiling with dmd built from this PR, so either those are different, or this PR doesn't fix the problem properly.

@dkorpel
Copy link
Contributor

dkorpel commented Jul 3, 2023

This baffles me. The failing test case works fine on my Ubuntu machine.

When I run the test, I get this warning:

/usr/bin/ld: warning: size of symbol `__dgliteral3' changed from 58 in {{RESULTS_DIR}}/runnable/test23786_0.o to 43 in {{RESULTS_DIR}}/runnable/test23786_0.o

I think the problem is that there's a symbol conflict in the delegates dmd creates for expression statements, since GNU assert() uses expression statements.

#include <assert.h>

void test23768()
{
    assert(3);
    assert(4); // emitted as __dgliteral3
}

void test24026()
{
    int x = 3;
    assert(x == 3); // also emitted as __dgliteral3
}

int main()
{
    test23768();
    test24026();
    return 0;
}

To work around it, perhaps use __check instead of assert.

@dkorpel
Copy link
Contributor

dkorpel commented Jul 3, 2023

Filed: Issue 24029 - ImportC: symbol name clash on statement expressions

@WalterBright
Copy link
Member Author

@dkorpel wow, nice catch. I never thought of that. You've saved me a lot of debugging effort!

@dkorpel
Copy link
Contributor

dkorpel commented Jul 3, 2023

Shall I rebase this to stable and merge or are you going to fix the failing cases (#15375 (comment), #15375 (comment)) in this PR?

@WalterBright
Copy link
Member Author

Shall I rebase this to stable and merge

Yes, since it is a regression. (I changed my mind.)

or are you going to fix the failing cases (#15375 (comment), #15375 (comment)) in this PR?

Yes, but as a separate PR. Should have a separate bugzilla for them, too. I can do that if you prefer.

@dkorpel
Copy link
Contributor

dkorpel commented Jul 4, 2023

Hmm, I can't check out this PR for some reason

 ! [rejected]              refs/pull/15375/head -> fix24026  (non-fast-forward)

@WalterBright
Copy link
Member Author

The PR is small enough it can just be cut&pasted into a new PR.

@dkorpel dkorpel changed the base branch from master to stable July 5, 2023 11:15
@dkorpel dkorpel force-pushed the fix24026 branch 2 times, most recently from 2b23a9b to 9713e34 Compare July 5, 2023 11:19
@dkorpel dkorpel merged commit 78dc82b into dlang:stable Jul 5, 2023
43 of 44 checks passed
@dkorpel
Copy link
Contributor

dkorpel commented Jul 5, 2023

Follow up issue filed: Issue 24031 - ImportC: DMD Rejects valid nested C initializers

@WalterBright WalterBright deleted the fix24026 branch July 6, 2023 02:30
@WalterBright
Copy link
Member Author

@dkorpel thanks!

@ibuclaw ibuclaw mentioned this pull request Jul 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants