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

Bad codegen with struct alignment and offsetof #2346

Closed
JohanEngelen opened this issue Sep 24, 2017 · 3 comments
Closed

Bad codegen with struct alignment and offsetof #2346

JohanEngelen opened this issue Sep 24, 2017 · 3 comments

Comments

@JohanEngelen
Copy link
Member

JohanEngelen commented Sep 24, 2017

Although the exact alignment requirements are unclear
The exact alignment requirements are clear since 2.075 (see forum post: http://forum.dlang.org/post/vzvxkabifknrzffycjao@forum.dlang.org), the following code should not crash but does.

struct UInt {
align(1):
    uint a;
}

struct Bug {
    ubyte one;
    UInt two;
}

align(1)
struct Align1UInt {
align(1):
    uint a;
}

struct BugAlign1 {
    ubyte one;
    Align1UInt two;
}

version(CRASH_DURING_COMPILATION) {
    struct A {
        Bug a;
        BugAlign1 b;
    }
}

void foobug(Bug* b) {
    b.two.a = 42;

    void* ptr = cast(void*)b;
    ptr += Bug.two.offsetof;
    assert( ptr == &b.two );
    assert( (cast(UInt*)ptr).a == 42 );
}

void fooalign1bug(BugAlign1 *b) {
    b.two.a = 42;

    void* ptr = cast(void*)b;
    ptr += BugAlign1.two.offsetof;
    assert( ptr == cast(void*)&b.two );
    assert( (cast(UInt*)ptr).a == 42 );
}

void main() {
    Bug b;
    foobug(&b);

    BugAlign1 b2;
    fooalign1bug(&b2);
}

DMD runs the code fine.

@JohanEngelen
Copy link
Member Author

We create the LLVM type wrong for Bug and BugAlign1 (they should be the same btw).

We almost output things correctly, I seems.
Slightly more complex case:

struct UInt {
align(2):
    uint a;
}

struct Bug {
    ubyte one;
    UInt two;
}

We output

%agrsize.UInt = type { i32 }
%agrsize.Bug = type { i8, [1 x i8], %agrsize.UInt }

(note that we already add the padding)
but I think it should be

%agrsize.UInt = type { i32 }
%agrsize.Bug = type <{ i8, [1 x i8], %agrsize.UInt }>

(angled brackets to indicate packed type, and that we ourselves take care of the padding)

@kinke
Copy link
Member

kinke commented Sep 24, 2017

I have a fix I think, PR is coming up shortly.

kinke added a commit to kinke/ldc that referenced this issue Sep 24, 2017
I probably introduced this regression lately. Back then, I didn't
account for container aggregates, which require that the LL struct is
packed if the overall D aggregate alignment is lower than a field's
natural alignment.
kinke added a commit to kinke/ldc that referenced this issue Sep 27, 2017
I probably introduced this regression lately. Back then, I didn't
account for container aggregates, which require that the LL struct is
packed if the overall D aggregate alignment is lower than a field's
natural alignment.
dnadlinger pushed a commit that referenced this issue Sep 28, 2017
I probably introduced this regression lately. Back then, I didn't
account for container aggregates, which require that the LL struct is
packed if the overall D aggregate alignment is lower than a field's
natural alignment.
@JohanEngelen
Copy link
Member Author

Fixed by #2347

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants