m68k-atari-mint-gcc icon indicating copy to clipboard operation
m68k-atari-mint-gcc copied to clipboard

STRUCTURE_SIZE_BOUNDARY: padding struct size

Open vinriviere opened this issue 10 months ago • 38 comments

This issue is about padding at the end of a struct to reach a certain size. I guess this is related to https://github.com/dxx-rebirth/dxx-rebirth/issues/695 (unverified).

Not to be confused with #20 about alignment of struct members.

$ cat s.c
struct s
{
        char a;
};

long n = sizeof(struct s);
$ gcc -m32 -S s.c -o -
        .long   1
$ gcc -m64 -S s.c -o -
        .quad   1
$ m68k-elf-gcc -S s.c -o -
        .long   1
$ m68k-atari-mint-gcc -S s.c -o -
        .long   2

So result is 1 everywhere except m68k-atari-mint-gcc.

This behaviour comes from this code: https://github.com/freemint/m68k-atari-mint-gcc/blob/gcc-13-mintelf/gcc/stor-layout.cc#L856-L868

Official GCC documentation: https://gcc.gnu.org/onlinedocs/gccint/Storage-Layout.html#index-STRUCTURE_005fSIZE_005fBOUNDARY

So if STRUCTURE_SIZE_BOUNDARY isn't defined, no specific padding is added at the end of the struct. A struct size of 1 or any odd size is allowed.

In GCC 13, STRUCTURE_SIZE_BOUNDARY is generally either undefined, or defined to 8-bit, which is the same. Only differences are arm.h (32), m68k/openbsd.h (16), and sh.h (variable).

Why m68k/openbsd.h chose a different default? No idea. Maybe some historical reason. Also, some old targets have disappeared from gcc sources, so old gcc versions might also be checked.

So what should be the correct setting for the new m68k-atari-mintelf target?

vinriviere avatar Aug 30 '23 20:08 vinriviere

My own opinion: Just use the default value of 1 byte, just like m68k-elf and virtually all other targets. Even if this will be different from the classic m68k-atari-mint.

Regarding to compatibility: Are there any 1-byte struct in the real world? No. Odd-sized structs? Probably not much, except C++ std::array. Remember that everything is statically linked on our target, so there is no ABI issue with shared libraries. There might be issues with existing static libraries, but as long as everything is rebuilt with the same toolchain, that's not a problem. Are there any possible issues regarding to the TOS/MiNT kernel API? I don't think so. If we detect some, they should really be fixed by using short where appropriate. As a real-world example: I've compiled EmuTOS with m68k-elf for years, and never notice any trouble.

Regarding to performance: Does it make any difference? I can't see how. That would have to be proven.

Any other potential issue? Anything specific when putting such structs into arrays?

vinriviere avatar Aug 30 '23 20:08 vinriviere

Note that ARM gcc has a -mstructure-size-boundary option to dynamically change that behaviour. However, it's deprecated.

vinriviere avatar Aug 30 '23 20:08 vinriviere

More complicated test: 3-bytes struct with a short (which must stay aligned).

struct s
{
        short a;
        char b;
};

Size is 4 bytes, with all compilers. So there is an implicit padding at the end, to respect the short alignment when that struct is stored in an array. I understand that such padding is only added at the end when it's useful for array alignment. So with default gcc settings, there will never be useless padding in structs of bytes. This looks optimal, in any case.

vinriviere avatar Aug 30 '23 21:08 vinriviere

There is also a hint in GCC's PCC_BITFIELD_TYPE_MATTERS documentation:

The other known way of making bit-fields work is to define STRUCTURE_SIZE_BOUNDARY as large as BIGGEST_ALIGNMENT. Then every structure can be accessed with fullwords.

So bitfields must also be checked.

A few quotes:

https://github.com/freemint/m68k-atari-mint-gcc/blob/gcc-13-mintelf/gcc/defaults.h#L1171-L1173 #define PCC_BITFIELD_TYPE_MATTERS false

https://github.com/freemint/m68k-atari-mint-gcc/blob/gcc-13-mintelf/gcc/config/elfos.h#L61-L65 /* Writing `int' for a bit-field forces int alignment for the structure. */

#ifndef PCC_BITFIELD_TYPE_MATTERS #define PCC_BITFIELD_TYPE_MATTERS 1 #endif

https://github.com/freemint/m68k-atari-mint-gcc/blob/gcc-13-mintelf/gcc/config/m68k/linux.h#L87-L89 /* For compatibility with linux/a.out */

#undef PCC_BITFIELD_TYPE_MATTERS

In my experimental m68k-atari-mintelf target, STRUCTURE_SIZE_BOUNDARY is currently unset (like 8) and PCC_BITFIELD_TYPE_MATTERS set to 1, just like m68k-elf. This is different from m68k-atari-mint.

Does bitfield compatibility matters for our current software? I can't believe that any OS API uses that.

vinriviere avatar Aug 30 '23 21:08 vinriviere

so there is no ABI issue with shared libraries

That still doesn't mean we can freely change it. We could change that now, but once the decision has been made, its too late. Code compiled with a compiler that uses different settings is not compatible, as i already found when i reverted your current setting to the old behaviour, but still had libraries lying around that were compiled with that compiler.

There is also a strong indication (seen at https://github.com/freemint/m68k-atari-mint-gcc/issues/20#issuecomment-1699687543) that we should not change that setting. There still seem to be places where it interferes. And getting an ICE is much worse than having to change (imho broken) code that relies on a certain packing. There is a good reason why bfd uses structures like https://github.com/freemint/m68k-atari-mint-binutils-gdb/blob/3ebda8ccd3e49f567ea71538ece8bde17d61a7b1/include/elf/external.h#L52-L67 when reading/writing that structure.

th-otto avatar Aug 31 '23 03:08 th-otto

to change (imho broken) code that relies on a certain packing

That's the problem here (and main argument/wish for keeping the default): you can't change that code easily, if you must have an array of chars of that size in a struct (the packed attribute is not going to help here). While with the "16-bit align problem" you can manually add padding, with this one you're done, you just have to use dirty hacks to get the desired behaviour.

So for me, I don't see a reason to make our lives more difficult with zero performance gain. That -malign-int is happening on our target only, mint-elf does support this and -malign-int without any ICEs.

EDIT: IMHO setting this to "16" and arguing that we've actually fixed something is wrong. We have just hidden the problem.

mikrosk avatar Aug 31 '23 06:08 mikrosk

I think we have already clarified that. Defaulting to -malign-int is a no-go. And if you use that in your project, as a global switch, you have to recompile all libs with that setting, including libcstdc++, mintlib, and whatnot. And i doubt that mintlib would work with that, without lots of changes.

That -malign-int is happening on our target only,

Hu? What makes you think so? That is not happening at all on our target unless you explicitly enable it.

So for me, I don't see a reason to make our lives more difficult with zero performance gain

And i don't see a reason to make our live more difficult by changing that setting, only because a single project cannot be compiled without it.

th-otto avatar Aug 31 '23 06:08 th-otto

I think we have already clarified that. Defaulting to -malign-int is a no-go.

Sure. But for usage in a single cpp file (wrapper) which doesn't interact with mintlib it is still a valid use case.

That -malign-int is happening on our target only,

Hu? What makes you think so? That is not happening at all on our target unless you explicitly enable it.

What I wanted to say that the "-malign-int problem is showing on our target only".

And i don't see a reason to make our live more difficult by changing that setting, only because a single project cannot be compiled without it.

How is it making your life difficult? You are not going to use -malign-int (so even if we immediately don't find the cause of that crash, it doesn't affect you) and as I mentioned above, we would be basically just hiding maybe a bigger problem which happens to show only with -malign-int and STRUCTURE_SIZE_BOUNDARY = 8.

mikrosk avatar Aug 31 '23 07:08 mikrosk

It is making life difficult because it changes ABI for no good reason. And because it might make the compiler ICE.

th-otto avatar Aug 31 '23 07:08 th-otto

And regarding shared libraries: we do have them, sort of. As either SLB or LDG. What will happen if you load such things into your app that was compiled by the mintelf toolchain, but the LDG was not?

th-otto avatar Aug 31 '23 07:08 th-otto

And regarding shared libraries: we do have them, sort of. As either SLB or LDG. What will happen if you load such things into your app that was compiled by the mintelf toolchain, but the LDG was not?

Oh. Yes. However, LDG/SLB are currently sparsely used. Are such odd-sized structs used in the real world? We should have a look.

It is making life difficult because it changes ABI for no good reason.

Good reason is to get rid of old and useless traditions. It seems that padding char structs to even size was a practice from old a.out days, while keeping structs to the smallest possible size is the new common practice. I don't see why we should keep old behaviour forever on the mint* platforms, specially after a major toolchain upgrade.

And except the LDG/SLB exception above, any ABI change isn't going to cause any trouble as long as everything is compiled with the same toolchain. As we believe that most users just use the toolchains and don't add third-party binary libraries, they shouldn't encounter any trouble.

vinriviere avatar Aug 31 '23 14:08 vinriviere

Are such odd-sized structs used in the real world? We should have a look.

Dunno how widely they are used, but the ones i compiled once are eg. now used in zView. The older version used *.ldg for that, but the result would be the same. OL had ldg libraries like polarssl.ldg and some others that are used by troll, litchi etc. Looking through all their public API would be difficult, and its easy to overlook something.

It seems that padding char structs to even size was a practice from old a.out days, while keeping structs to the smallest possible size is the new common practice.

I don't think it has anything to do with a.out or elf, its just a matter of the m68k architecture. IINW, it might have to do with the bus architecture, which accesses 16 bit even if you read only a byte.

I don't see why we should keep old behaviour forever on the mint* platforms, specially after a major toolchain upgrade.

And i still see no reason to change that. It does not give any benefit, only gives troubles. And it doesn't even help in the case of d2x-rebirth, which still cannot be compiled.

And except the LDG/SLB exception above, any ABI change isn't going to cause any trouble as long as everything is >compiled with the same toolchain.

You will have the same trouble when reading/writing such structures to disk, or to communicate over the network.

So what do you expect from such an ABI change? Better compatibility with other archictures? No, that is not the case. And why should we care about other architectures. Better performance? No, because that does not change at all.

Correct me if i'm wrong, but initially that ABI change was introduced because you added m68kemb.h to the target files. That already introduced some ABI changes that are certainly not wanted.

th-otto avatar Sep 01 '23 04:09 th-otto

And to compare the definitions from m68kemb.h:

  • PTRDIFF_TYPE, SIZE_TYPE, DEFAULT_PCC_STRUCT_RETURN: defined the same in mint.h. No problem.

  • FUNCTION_VALUE: defined to LIBCALL_VALUE, which in turn calls m68k_libcall_value. This would be wrong, as it is not redefined in mint.h, and would result in the linux/svr4 abi of returning float values returned in FPU registers.

  • LIBCALL_VALUE: see above.

  • FUNCTION_VALUE_REGNO_P, FUNCTION_VALUE: same problem as above

  • NEEDS_UNTYPED_CALL: result of the definitions above, using more than one register for FUNCTION_VALUE_REGNO

  • TARGET_OS_CPP_BUILTINS, LIB_SPEC, STARTFILE_SPEC: redefined in mint.h again. No problem.

So all that definitions were actually wrong for our target, or redefined in mint.h. That only leaves the definition of PCC_BITFIELD_TYPE_MATTERS, which you are keen to keep. To be compatible with what, an embedded target? Which exactly should that be?

th-otto avatar Sep 01 '23 05:09 th-otto

And i still see no reason to change that. It does not give any benefit, only gives troubles. And it doesn't even help in the case of d2x-rebirth, which still cannot be compiled.

But if/when that bug will be identified and fixed, it will be too late to change the ABI.

I guess I have nothing new to add, for me this is just convenience, that's the benefit for myself at least. I can take a project source code and compile it. Without this change I will have to probably drop that project because I didn't want to spend too much time with it.

Maybe next time you or Vincent or someone else find a project which depends on non-padded struct size and again, it will be too late to change anything.

mikrosk avatar Sep 02 '23 15:09 mikrosk

For the sake of completeness, that bug has been identified and fixed: https://github.com/freemint/m68k-atari-mint-gcc/issues/22#issuecomment-1705697651. Now https://github.com/dxx-rebirth/dxx-rebirth/issues/695 compiles as expected with -malign-int and STRUCTURE_SIZE_BOUNDARY = 8. This wont be the case with STRUCTURE_SIZE_BOUNDARY == 16 and there wont be another way to fix it except rewriting the original code.

mikrosk avatar Sep 04 '23 21:09 mikrosk

I did some tests check what code differences there might me. Attached you will find an archive, where mintlib was compiled a) with a compiler that had PCC_BITFIELD_TYPE_MATTERS, and b) with a compiler that had STRUCTURE_SIZE_BOUNDARY=16. For the whole mintlib, only 5 object files actually differed.

There are also disassemblies of the object files, and a diffs.txt where the differences are more easily visible. Actually the difeferences are rather small (two byte instead of one word moves mostly), and i still cannot explain the crashes i am encountering when

  • compiling mintlib with the PCC_BITFIELD_TYPE_MATTERS compiler
  • install it
  • compile a test program that links to -lreadline, using the compiler with STRUCTURE_SIZE_BOUNDARY=16.

But fact is, that this program crashes somewhere in tcgetattr, one of the files that differ in mintlib.

And if nothing else, the code with the new setting is a bit less optimal.

bitfield.zip

th-otto avatar Sep 14 '23 14:09 th-otto

Very good investigation @th-otto. I plan to also have a look, without hurry. I'm really curious to see what's behind that issue.

vinriviere avatar Sep 14 '23 14:09 vinriviere

Phew, found the problem with the crash. It was not directly caused by different structure-layout, but a side-effect of the compilers generating slightly different code. The actual problem was the generation of jump tables for switch statements. In ioctl.o, the 2nd jump-table looked like

 14e:   323b 3808       movew %pc@(158 <__ioctl+0x158>,%d3:l),%d1
 152:   4efb 1002       jmp %pc@(156 <__ioctl+0x156>,%d1:w)
 156:   0000 0120       orib #32,%d0

Note the difference between the 158 and the 156. That was caused by gcc emitting an align directive before the table, which results in a mismatch for the offset expected to be 2 in the ASM_RETURN_CASE_JUMP macro.

Fixed now, and the crashes are gone.

But it leaves still leaves the question whether we should use PCC_BITFIELD_TYPE_MATTTERS. It seems to produce inefficient code when copying small structures (doing single-byte moves), like in https://github.com/freemint/mintlib/blob/4440b47d80cd43e88258c8939b0d7497f4a88420/unix/ioctl.c#L210

th-otto avatar Sep 15 '23 06:09 th-otto

That was caused by gcc emitting an align directive before the table, which results in a mismatch for the offset expected to be 2 in the ASM_RETURN_CASE_JUMP macro.

Damn! I remember that I already hit that issue with GCC 4, and fixed it by redefining the ASM_RETURN_CASE_JUMP macro. I didn't keep that patch for the mintelf target, and mentally noted: to be checked later. Then of course I forgot. I really wonder how this can work on other m68k platforms. Maybe related to that swbeg directive, or different sections, etc. Evil parameter. I guess that we would need to use a big switch with many labels to produce a simple testcase.

But it leaves still leaves the question whether we should use PCC_BITFIELD_TYPE_MATTTERS. It seems to produce inefficient code when copying small structures (doing single-byte moves),

Ah, that's the interesting point.

vinriviere avatar Sep 15 '23 09:09 vinriviere

I really wonder how this can work on other m68k platforms.

Yes, that's really strange.

Maybe related to that swbeg directive,

Not directly to the swbeg directive, but m68kelf.h redefines ASM_OUTPUT_BEFORE_CASE_LABEL. If that is not done, The definition from elfos.h https://github.com/freemint/m68k-atari-mint-gcc/blob/3580ef9e4154fdef4eedf25ed6c299059b5fe84c/gcc/config/elfos.h#L145-L146 is used (2 there is a log2 value, so that aligns to 4 byte)

Since both are elf specific, that also explains why we didn't encounter that in a.out toolchains.

I guess that we would need to use a big switch with many labels to produce a simple testcase.

I've attached a preprocessed version of ioctl.c that triggered the problem. But the problem is that there is a 50/50 chance that it will still work, no matter how many cases there are. There must also be something different in the settings between compilers; gcc-7 and gcc-4 do not emit a jumptable for that example, but a series of compares instead.

ioctl.c.txt

th-otto avatar Sep 16 '23 06:09 th-otto

For memories, regarding this off-topic question:

Not directly to the swbeg directive, but m68kelf.h redefines ASM_OUTPUT_BEFORE_CASE_LABEL. If that is not done, The definition from elfos.h

It has been discussed in https://github.com/freemint/m68k-atari-mint-gcc/issues/34. Finally, root cause was a GCC bug in the ASM_RETURN_CASE_JUMP macro. And the solution was to force ADDR_VEC_ALIGN to 0.

vinriviere avatar Nov 20 '23 23:11 vinriviere

Now I would like to precisely document the effects of STRUCTURE_SIZE_BOUNDARY. Key point is that, by default, structs are aligned and padded to the size of the biggest member. But there are exceptions.

  • When STRUCTURE_SIZE_BOUNDARY is undefined, or set to 8 bit, there is no minimum struct size. So a struct can be as small as a single byte. This is the case on most targets, with the notable exception of our old m68k-atari-mint gcc.

Testcase: align.c

struct s
{
	char a;
} g;

int size = sizeof(struct s);

void f(void)
{
	g.a = 1;
}

m68k-elf-gcc -S align.c -o - -Os -fomit-frame-pointer

#NO_APP
	.file	"align.c"
	.text
	.align	2
	.globl	f
	.type	f, @function
f:
	move.b #1,g
	rts
	.size	f, .-f
	.globl	size
	.data
	.align	2
	.type	size, @object
	.size	size, 4
size:
	.long	1
	.globl	g
	.section	.bss
	.type	g, @object
	.size	g, 1
g:
	.zero	1

For clarity, I will focus on the following characteristics: struct size, struct alignment, initialization code. In this case:

Size: 1, equal to single member size
Alignment: unspecified (1, equal to size)
	move.b #1,g

So that's optimal. Same result with m68k-elf-gcc and m68k-linux-gcc (but not m68k-atari-mint).

Next: turn the single member to short.

struct s
{
        short a;
} g;

int size = sizeof(struct s);

void f(void)
{
        g.a = 1;
}

Result:

Size: 2, equal to single member size
Alignment:  2, equal to size
		move.w #1,g

This happens on all compilers: m68k-elf, m68k-linux, and even m68k-atari-mint.

So putting a short into the struct has 2 effects:

  • it aligns the struct to a multiple of 2 bytes
  • it allows usage of move.w to access the member. That's perfectly fine.

Now a mix: char + short + char

struct s
{
        char a;
        short b;
        char c;
} g;

int size = sizeof(struct s);

void f(void)
{
        g.a = 1;
        g.b = 2;
        g.c = 3;
}

Result with all compilers: m68k-elf, m68k-linux, and even m68k-atari-mint:

Size: 6
Alignment: 2, equal to biggest member size
	move.b #1,g
	move.w #2,g+2
	move.b #3,g+4

We can see that:

  • the struct itself and the internal short are aligned on 2 bytes
  • padding is added to reach a multiple of 2 bytes

Again, same test with a long:

struct s
{
        char a;
        long b;
        char c;
} g;

int size = sizeof(struct s);

void f(void)
{
        g.a = 1;
        g.b = 2;
        g.c = 3;
}

First, I compile specifically with -malign-int to mimic other processors standards: m68k-elf-gcc -S align.c -o - -Os -fomit-frame-pointer -malign-int

Result with all compilers: m68k-elf, m68k-linux, and even m68k-atari-mint:

Size: 12
Alignment: 4, equal to biggest member size
	move.b #1,g
	moveq #2,%d0
	move.l %d0,g+4
	move.b #3,g+8

We can see that:

  • the struct itself and the internal long are aligned on 4 bytes
  • padding is added to reach a multiple of 4 bytes
  • move.l is used

So, as a general rule, the size of the biggest member dictates 2 things:

  • struct alignment
  • struct padding

Now same test, but this time without -malign-int. Remember that, by default on 68000, 4-byte int alignment is relaxed to 2 bytes only thanks to the BIGGEST_ALIGNMENT definition.

m68k-elf-gcc -S align.c -o - -Os -fomit-frame-pointer

Result with all compilers: m68k-elf, m68k-linux, and even m68k-atari-mint:

Size: 8
Alignment: 2
	move.b #1,g
	moveq #2,%d0
	move.l %d0,g+2
	move.b #3,g+6

We can see that, by default, alignment and padding are restricted to 2 bytes, even for the long type. This is the optimal layout for 68000, where 4-byte alignment isn't required. move.l can still be used.

And finally, last test: just a single byte, but compiled with the old m68k-atari-mint-gcc:

struct s
{
	char a;
} g;

int size = sizeof(struct s);

void f(void)
{
	g.a = 1;
}

Result:

Size: 2
Alignment: unspecified (2, equal to size)
	move.b #1,_g

So for the very special case of m68k-atari-mint, due to STRUCTURE_SIZE_BOUNDARY=16, every struct is handled as if it would have contained at least one field of type 'short', even if it only contains bytes. This has the effect of padding all char-only structs to a size multiple of 2 bytes. While this is perfectly legal regarding to the C standard (structs may be padded), this is inconsistent with actual gcc implementation of all main targets.

vinriviere avatar Nov 21 '23 00:11 vinriviere

Thank you Vincent for the complete overview. It would seem that the m68k-atari-mint is really the only black sheep when handling single chars.

mikrosk avatar Nov 21 '23 07:11 mikrosk

Now let's look at the effect of __attribute__((packed)).

struct s
{
        char a;
        short b;
} __attribute__((packed)) g;

int size = sizeof(struct s);

void f(void)
{
        g.a = 1;
        g.b = 2;
}

m68k-elf-gcc -S align.c -o - -Os -fomit-frame-pointer

Size: 3, unaligned.
	move.b #1,g
	clr.b g+1
	move.b #2,g+2

We can see that __attribute__((packed)) has the following effects:

  • all padding has disappeared, both in the middle of the struct and at the end.
  • size is exactly the sum of the sizes of the members, and can be odd.
  • alignment of members isn't respected anymore (including short and longs)
  • to deal with potential misalignment, gcc never uses move.w or move.l. Instead, it accesses all members, including short and longs, with a series of move.b.

So __attribute__((packed)) is evil: on 68000, it introduces slowness when accessing short/long members. To be used only when it is known that the data will actually be misaligned, so move.w/l can't be used.

However, I noticed a few interesting points.

  • m68k-linux allows misaligned access (allowed on 68020+), so on that target packed shorts are still accessed with move.w. But that's a special case.

  • -malign-int has no effect on packed structs. This makes sense, as 'packed' means removing all alignment and padding.

  • Our old m68k-atari-mint behaves exactly like m68k-elf in this regard. Packed structs are still packed, and 1-byte or 3-byte sizes are honored. This mean that defining STRUCTURE_SIZE_BOUNDARY has no effect on packed structs.

As a conclusion:

  • m68k-atari-mint can produce 1-byte or 3-byte structs, but that requires usage of __attribute__((packed)), while this isn't required on other targets.
  • Forcing __attribute__((packed)) has no adverse effect if the struct only contains bytes. But if the struct contains shorts and longs, access will be slow.

So using __attribute__((packed)) can't be used everywhere.

vinriviere avatar Nov 22 '23 02:11 vinriviere

I'd also explicitly mention that the effect of STRUCTURE_SIZE_BOUNDARY=16 (one char in a struct being two bytes big) cannot be mitigated by __attribute__((packed)), the size will be two bytes as before.

EDIT: Oh wait, I've confused the aligning on int boundaries vs. padding the structs. I'll take a closer look (again) which is which.

mikrosk avatar Nov 22 '23 08:11 mikrosk

I'd also explicitly mention that the effect of STRUCTURE_SIZE_BOUNDARY=16 (one char in a struct being two bytes big) cannot be mitigated by __attribute__((packed)), the size will be two bytes as before.

@mikrosk I've just saw your edit. Indeed, the above quote is wrong. Actually, __attribute__((packed)) can mitigate the problem of the evil STRUCTURE_SIZE_BOUNDARY=16. So __attribute__((packed)) is a perfectly valid and safe workaround for 1-byte struct: the struct size will be 1 byte, as expected.

This was the main point of my previous message: __attribute__((packed)) have precedence over STRUCTURE_SIZE_BOUNDARY=16.

vinriviere avatar Nov 22 '23 11:11 vinriviere

Now I know where the confusion came from: I mention in https://github.com/dxx-rebirth/dxx-rebirth/issues/695#issuecomment-1400986448 that __attribute__((packed)) indeed did not help. However the reason why it didn't help was that I was packing callsign_t here: https://github.com/dxx-rebirth/dxx-rebirth/blob/a6161c031a5b659073695138d2d848b831720078/common/main/player-callsign.h#L20 while the wrong value came from std::array size (which I, naturally, can't pack).

mikrosk avatar Nov 22 '23 11:11 mikrosk

I see a remark about STRUCTURE_SIZE_BOUNDARY inside the documentation for PCC_BITFIELD_TYPE_MATTERS.

[Instead of setting PCC_BITFIELD_TYPE_MATTERS=1] The other known way of making bit-fields work is to define STRUCTURE_SIZE_BOUNDARY as large as BIGGEST_ALIGNMENT. Then every structure can be accessed with fullwords.

By reading that, it seems there are only 2 options to make bitfields work:

  • set PCC_BITFIELD_TYPE_MATTERS=1
  • or set STRUCTURE_SIZE_BOUNDARY to BIGGEST_ALIGNMENT.

But this is wrong. m68k-linux is the counter example. It explicitly undefs PCC_BITFIELD_TYPE_MATTERS, so it defaults to false/0. And it doesn't define STRUCTURE_SIZE_BOUNDARY at all. Result is that bitfield type doesn't matter, while 1-sized structs are still allowed.

So there are actually 3 different settings in the wild:

  • m68k-elf uses only PCC_BITFIELD_TYPE_MATTERS=1
  • m68k-linux uses only PCC_BITFIELD_TYPE_MATTERS=0
  • m68k-atari-mint uses PCC_BITFIELD_TYPE_MATTERS=0 and STRUCTURE_SIZE_BOUNDARY=BIGGEST_ALIGNMENT.

vinriviere avatar Dec 05 '23 00:12 vinriviere

Can you clarify what they mean by "making bit-fields work" ?

mikrosk avatar Dec 05 '23 07:12 mikrosk

Can you clarify what they mean by "making bit-fields work" ?

This is the good question. I can't see what this sentence refers to. This is obviously wrong, as m68k-linux is the counter-example. Maybe there's something related to alignment, and m68k-linux isn't a good counter-example as it doesn't require strict alignment?

Maybe that implies that bitfields are always accessed using move.w, so they require alignment? Clearly no, we don't see such thing in the various testcases.

So I really don't see any good technical reason to increase STRUCTURE_SIZE_BOUNDARY. Maybe it existed at some point, but I can't see any clue in current code.

The only reason to increase STRUCTURE_SIZE_BOUNDARY seems to keep binary compatibility with the layout of some old compiler.

vinriviere avatar Dec 05 '23 14:12 vinriviere