[Solved] EXT2 strangeness, again :(

[mikegonta]

Here's something for you to read. And, most importantly, this part.

In that discussion, James-M is the only guy who really knows something about C compilers - he writes them for a living.

That's why I personally would mark every struct you need packed manually, as packed.

And I agree, but why mark a struct packed that is already packed.
I should point out that

Please don't pack already "packed" structs.

is a redundant statement (and not advice) much along the lines of "If it's not broken, don't fix it" and we all know how misguided that one is.

So thanks for the read, I'm going to repeat now - "That's like the most misguided C advice I've ever heard on this forum from person who has
little to no idea about how C works."

It would probably be better to present your reasons why you disagree and engage in the discussion, rather than seek advice from these forums.
The only thing worst than unsolicited advice is unqualified expert testimony.

[mikegonta]

is there a way to tell your C compiler :
I don't care about how this struct looks in memory. You can reverse members order, skip those ever written to or read.

You can't if you want to interface (at a high level) with on device structs; for example copying between the two. Using unsigned char
array is portable with less chance that the compiler will do something unexpected at the cost of giving up the simplicity of struct
assignment and notation and requiring the use of possibly error prone #defines.

Optimise using Lto and do it according to my arch.

Ultimately every compile is arch specific, the main concern is how much the code has to be changed to adapt to a different arch.
LTO is promising, but not a panacea.

[matt11235]

And I agree, but why mark a struct packed that is already packed.

Just because it is packed on one platform doesn't mean that it is packed for every platform.

[mikegonta]

And I agree, but why mark a struct packed that is already packed.

Just because it is packed on one platform doesn't mean that it is packed for every platform.

Agreed.
But on-device structs packed or otherwise are not platform portable anyways.
So, if your arch specific platform requires packing, packing is available.
And off course it's best not to second guess the compiler, but then again it's probably best not to guess when it comes to programming.

[Kevin]

Why would a filesystem driver be platform specific? This doesn't make any sense at all.

Also, can you please explain how using a char array instead of a proper struct fixes endianess issues? I could see how a char array instead of an int can be used to do this (in a rather tedious way compared to the usual conversion functions), but that's not related to structs at all.

[mikegonta]

Why would a filesystem driver be platform specific? This doesn't make any sense at all.

Of course it's not the file system driver that's platform specific but rather how the platform deals with the on-device structs.

Also, can you please explain how using a char array instead of a proper struct fixes endianess issues? I could see how a char array
instead of an int can be used to do this (in a rather tedious way compared to the usual conversion functions), but that's not
related to structs at all.

Here's an example taken from FATfs a generic cross platform FAT/exFAT file system driver used mainly for embedded devices.

Code: Select all

/* FatFs refers the members in the FAT structures as byte array instead of
/ structure members because the structure is not binary compatible between
/ different platforms */

#define BS_JmpBoot			0		/* x86 jump instruction (3-byte) */
#define BS_OEMName			3		/* OEM name (8-byte) */
#define BPB_BytsPerSec		11		/* Sector size [byte] (WORD) */
#define BPB_SecPerClus		13		/* Cluster size [sector] (BYTE) */
#define BPB_RsvdSecCnt		14		/* Size of reserved area [sector] (WORD) */
#define BPB_NumFATs			16		/* Number of FATs (BYTE) */

and here is one of several functions which access the byte arrays in an endian neutral way.

Code: Select all

#if _FS_EXFAT
static
QWORD ld_qword (const BYTE* ptr)	/* Load an 8-byte little-endian word */
{
	QWORD rv;

	rv = ptr[7];
	rv = rv << 8 | ptr[6];
	rv = rv << 8 | ptr[5];
	rv = rv << 8 | ptr[4];
	rv = rv << 8 | ptr[3];
	rv = rv << 8 | ptr[2];
	rv = rv << 8 | ptr[1];
	rv = rv << 8 | ptr[0];
	return rv;
}
#endif

And of course internally structs are used and are the preferred "C" way.

[mikegonta]

Please don't pack already "packed" structs.

Is it guaranteed that without "packed" the compiler will not add padding? Isn't it implementation specific? Granted most likely no
sane current compiler would add padding, but do you want to rely on that?
Even if it were guaranteed, and this is my main point, I'd mark it as packed to explicitly convey my intent. If the compiler is bad
and does something stupid then you should fix the compiler.
I guess the question is, why does GCC do a complex and unnecessary "memmove" instead of "rep movs"?

Actually, with -march=i486, memmove also uses rep movsd. The extra code and complexity is for aligning the loop. The big problem
is that if you pack already packed structs GCC can't/won't assume alignment.

Can't you tell the compiler the alignment?

Apparently you can, (according to the linked article) which I can verify, by using __attribute__((packed, aligned(x))) where x is the
required byte alignment, in this case 4. With this there are no unwanted/unneeded side effects.
Start packing to your heart's content.
Case closed!

[dozniak]

Start packing to your heart's content.
Case closed!

Are you finally converted in the ways of GCC compiler then?

[mikegonta]

Start packing to your heart's content.
Case closed!

Are you finally converted in the ways of GCC compiler then?

Absolutely not!
You still cannot substitute __attribute__ ((__packed__)) for __attribute__ ((__packed__, __aligned__())) in all cases.
If the struct is already packed all is good, the same as not using __attribute__ ((__packed__)) in the first place.
However, if the struct needs packing __attribute__ ((__packed__, __aligned__())) will cause GCC to transfer (in the case of
assignment) a number of bytes divisible by the alignment which could possibly cause overwriting of the on-device struct.
GCC effectively puts the padding at the end while preserving the internal structure.

[dozniak]

However, if the struct needs packing __attribute__ ((__packed__, __aligned__())) will cause GCC to transfer (in the case of
assignment) a number of bytes divisible by the alignment which could possibly cause overwriting of the on-device struct.
GCC effectively puts the padding at the end while preserving the internal structure.

Erm, this is documented behavior for aligned()

Doing this can often make copy operations more efficient, because the compiler can use whatever instructions copy the biggest chunks of memory when performing copies to or from the variables or fields that you have aligned this way.

When you specify alignment gcc will use that as size-hint for copy operations, because structure will be aligned on these boundaries, regardless of packed attribute.

If you want structure to be packed but also to be exactly as "on-device struct" (whatever that is and wherever you got it from) DO NOT USE ALIGNED!

Clear now?