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

codegen: bitcast to expanded struct type when accessing its fields #209

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

Hyxogen
Copy link
Contributor

@Hyxogen Hyxogen commented Feb 9, 2024

Cheerp, for structs, expands other structs that are in the first member position. During this expansion, for technical reasons, it explicitly already adds the padding to the expanded struct. So for structs like this:

        struct foo {
                uint16_t a;
                //implicit padding of 2 bytes
                uint32_t b;
        };

        struct bar {
                struct foo f;
        }

It will actually become something like this:

        struct foo {
                uint16_t a;
                //implicit padding of 2 bytes
                uint32_t b;
        };

        struct bar {
                uint16_t a;
                uint8_t _padding[2];
                uint32_t b;
        }

However, accesses to the base struct's fields were not aware of this extra field being added, and falsely assuming that, for bar, b would still be in the second position like it is for foo.

The linear memory layout of these two types is, however, still exactly the same. The only difference for LLVM is that the implicit padding is already there.

Fixed this by bitcasting to the expanded struct type before trying to access its fields.

Closes: leaningtech/cheerp-meta#118

I've checked for the cheerp target, and it doesn't seem to be adding the padding like we might thought it would.

Please merge leaningtech/cheerp-utils#66 before this PR

@Hyxogen Hyxogen requested a review from yuri91 February 9, 2024 14:08
Cheerp, for structs, expands other structs that are in the first member
position. During this expansion, for technical reasons, it explicitly
already adds the padding to the expanded struct. So for structs like
this:

        struct foo {
                uint16_t a;
                //implicit padding of 2 bytes
                uint32_t b;
        };

        struct bar {
                struct foo f;
        }

It will actually become something like this:

        struct foo {
                uint16_t a;
                //implicit padding of 2 bytes
                uint32_t b;
        };

        struct bar {
                uint16_t a;
                uint8_t _padding[2];
                uint32_t b;
        }

However, accesses to the base struct's fields were not aware of this
extra field being added, and falsely assuming that, for `bar`, `b` would
still be in the second position like it is for `foo`.

The linear memory layout of these two types is, however, still exactly
the same. The only difference for LLVM is that the implicit padding is
already there.

Fixed this by bitcasting to the expanded struct type before trying to
access its fields.

Closes: leaningtech/cheerp-meta#118
@Hyxogen Hyxogen force-pushed the fix-struct-base-access branch from 413f41e to 46e0883 Compare February 9, 2024 14:18
@Hyxogen Hyxogen requested a review from yuri91 February 9, 2024 14:18
@yuri91 yuri91 merged commit f6df675 into master Feb 9, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overwrite unexpected struct filed
2 participants