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

was change in structure padding intended? #17640

Closed
mithrendal opened this issue Aug 14, 2022 · 4 comments
Closed

was change in structure padding intended? #17640

mithrendal opened this issue Aug 14, 2022 · 4 comments

Comments

@mithrendal
Copy link

beginning with emcc 3.1.16 we see structure padding was changed from 4 bytes alignment to 8 bytes alignment.

was this done by intention? Normally 8 bytes alignment is for 64bit, but emsdk is still 32 bit ... no?
Can you tell us why the 4 bytes alignment was not sufficient anymore? Is emsdk maybe going 64 Bit soon ?

e.g.

struct SnapshotHeader {    
    // Magic bytes ('V','A','S','N','A','P')
    char magic[6];
    
    // Version number (major.minor.subminor['b'beta])
    u8 major;
    u8 minor;
    u8 subminor;
    u8 beta;

    //<--- here after byte 10, up to 3.1.15 emcc inserted a padding of 2 Bytes
    // to get the following struct aligned to an address divisable by 4 

    //<--- here after byte 10, 3.1.16 emcc and later inserts a padding of 6 Bytes
    // to get the following struct aligned to an address divisable by 8

    // this is the following struct
    Thumbnail screenshot;
};

code extract from vAmigaWeb emulator see the https://vamigaweb.github.io/doc/about.html

@juj
Copy link
Collaborator

juj commented Aug 15, 2022

What is the type Thumbnail?

This reads like a bug if Thumbnail only needs to be aligned to 4, but now is getting aligned to 8 (independent of targeting Wasm as 64bit or 32bit)

@mithrendal
Copy link
Author


struct Thumbnail {
    
    // Image size
    i32 width, height;
    
    // Raw texture data
    u32 screen[(HPIXELS / 2) * (VPIXELS / 1)];
    
    // Creation date and time
    time_t timestamp;
    
    // Takes a screenshot from a given Amiga
    void take(Amiga &amiga, isize dx = 2, isize dy = 1);
};

struct SnapshotHeader {
    
    // Magic bytes ('V','A','S','N','A','P')
    char magic[6];
    
    // Version number (major.minor.subminor['b'beta])
    u8 major;
    u8 minor;
    u8 subminor;
    u8 beta;
    
    // Preview image
    Thumbnail screenshot;
};

https://github.com/vAmigaWeb/vAmigaWeb/blob/fbd0907e01ce998972d4afff6469e703ccc319ef/Emulator/Media/Snapshot.h#L17

@juj
Copy link
Collaborator

juj commented Aug 15, 2022

Given the time_t in the struct, this might then be caused by #17401 . That change now necessitates that Thumbnail must be 8-byte aligned.

@mithrendal
Copy link
Author

time_t is now 64 Bit. Ok that perfectly explains it. Thank you very much for clarification. I close the issue then.

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

No branches or pull requests

2 participants