ht: Fix arm64 compilation

[rdoeffinger (Reimar Döffinger)]

This simple additional patch makes it compile, and as far as can tell work, on AppleSilicon devices: ​https://github.com/sebastianbiallas/ht/pull/31 Full diff:

diff --git a/httag.h b/httag.h
index 7f5da1c..5cbd500 100644
--- a/httag.h
+++ b/httag.h
@@ -71,7 +71,7 @@ struct ht_tag_flags {
 struct ht_tag_flags_s {
        char bitidx;
        const char *desc;
-} PACKED;
+};
 
 /* GROUP-TAG */
 #define HT_TAG_GROUP                           0x03

[ryandesign (Ryan Carsten Schmidt)]

I don't know the consequences of changing this struct from packed to not packed so let's wait to see what the developers say.

[jmroot (Joshua Root)]

This will change the struct layout, which would break binary compatibility with anything using it, but fortunately it looks like this header is not intended to be used by anything but this program.

The packed declaration is presumably to avoid wasting memory with a "hole" in the struct due to alignment. It's traditional to declare struct members in order from largest to smallest alignment to avoid this problem while also avoiding unaligned accesses, which are usually less efficient even if the architecture allows them.

[rdoeffinger (Reimar Döffinger)]

I agree that ideally we will get a response from upstream, just not sure how active they are.

So I'll provide a basic explanation what I think goes on here.

A lot of the structs in this file are meant to be serialized out into buffers.

HT_TAG_BUFOP adds functions like flush() that memcpy them and also special serialization functions in the .cc file. For that you'd want them packed, because both you'd not want random data from the padding to leak out and also for the size and layout to not depend on architecture/OS/compiler.

However in case of ht_tag_flags_s I suspect that PACKED was added by pure accident.

Why? First of all, PACKED was added in 5c0c87f2 along with all other structs in the file, so likely it was not closely reviewed. Second of all, this struct contains no HT_TAG_BUFOP, and it is the only one containing a pointer type. And it's not really possible to sensibly serialize a pointer to a buffer (without some special logic, which does not exist here).

I think, it is rather the opposite, the ht_tag_flags_s are a mapping between the values being serialized out and the strings they stand for, so they are explicitly meant to live only in the binary, not the buffers that are created here.

Thus I do think the change makes sense, however I agree it is sensible to wait a while and see if we can get an upstream response. I guess the only counter-point is, for AppleSilicon devices it currently does not compile at all, so applying it only for that case does not really make anything worse even if I'm wrong...

[rdoeffinger (Reimar Döffinger)]

It's been a month, could you consider this change, independent from if and when upstream might get around to look into it?

[ryandesign (Ryan Carsten Schmidt)]

Replying to rdoeffinger:

This simple additional patch makes it compile, and as far as can tell work, on AppleSilicon devices

It would have been nice if you would have shown us the error message you received before applying this change. Another ticket has been filed about an arm64 build failure in #71286. Is that the same error you saw?

[TheRealKeto (Keto)]

Replying to ryandesign:

Replying to rdoeffinger:

This simple additional patch makes it compile, and as far as can tell work, on AppleSilicon devices

It would have been nice if you would have shown us the error message you received before applying this change. Another ticket has been filed about an arm64 build failure in #71286. Is that the same error you saw?

Homebrew's notes when applying the patch from upstream describe fixing #71286 and was added to fix building on Apple Silicon.

​https://github.com/Homebrew/homebrew-core/blob/master/Formula/h/ht.rb#L30-L38

[ryandesign (Ryan Carsten Schmidt)]

In 0649cd876cf9341107883482f9488ba63072e207/macports-ports (master):

ht: Don't pack struct ht_tag_flags_s

Fixes build failure on arm64.

Closes: #67790
Closes: #71286