Fix struct align/pack for MSVC

rasmusbonnedal commented 7 months ago

Description

This PR fixes a build error due to MSVC not supporting __attribute__((packed, aligned(1))) that GCC and Clang use.

Tests

I wrote a static_assert which checks the sizeof a struct which contains fields int, char, int to ensure it is not padded. I have verified this test fails (on both GCC and MSVC) if the packed attribute/pragma is not present.

Checklist:

[x] I have read the contribution guidelines.
[x] I have updated the documentation, if applicable.
[x] I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
[x] My code follows the prevailing code style of this project. If I haven't already run clang-format before submitting, I definitely will look at the CI test that runs clang-format and fix anything that it highlights as being nonconforming.

lgritz commented 7 months ago

@steenax86 Does this look ok to you?

AlexMWells commented 7 months ago

Looking at it now. Operationally is appears fine, but I'm working through some other options, give me an hour...

AlexMWells commented 7 months ago

Some experiments here: https://www.godbolt.org/z/PfTP6G4eK

So it appears the

#pragma pack(push, 1)
#pragma pack(pop)

works across all compilers. So although it's not as desirable as the __attribute__((packed)) it works on the compilers of interest and could reduce customization points. And the alignment portion of the original __attribute__((packed, aligned(1))) could be changed to C++11 feature alignas(1) right after the structs declaration, which appears to work for all compilers as well.

So suggest something non-conditional on compilers like:

#    define OSL_PACK_STRUCTS_BEGIN __pragma(pack(push, 1))
#    define OSL_PACK_STRUCTS_END __pragma(pack(pop))

and

OSL_PACKED_STRUCT_BEGIN
template<int IndexT, typename TypeT> struct alignas(1) PackedArg {
    explicit PackedArg(const TypeT& a_value) : m_value(a_value) {}
    TypeT m_value;
};
OSL_PACKED_STRUCT_END

OSL_PACKED_STRUCT_BEGIN
template<int... IntegerListT, typename... TypeListT>
struct alignas(1)
    PackedArgsBase<std::integer_sequence<int, IntegerListT...>, TypeListT...>
    : public PackedArg<IntegerListT, TypeListT>... {
    explicit PackedArgsBase(const TypeListT&... a_values)
        // multiple inheritance of individual components
        // uniquely identified by the <Integer,Type> combo
        : PackedArg<IntegerListT, TypeListT>(a_values)...
    {
    }
};
OSL_PACKED_STRUCT_END

with additional checks


static_assert(alignof(PackedArgs<int, char, int>)
                  == 1,
              "PackedArgs<> type is not aligned to 1");

Just to keep the alignment less compiler specific, and less conditional macros.

rasmusbonnedal commented 7 months ago

@AlexMWells Thanks for the improvement!

I had to make a slight adjustment because Clang didn't support __pragma. And I couldn't get

#define OSL_PACK_STRUCTS_BEGIN #pragma pack(push, 1)

to work. I found the construct _Pragma("pack(push, 1)") which I could #define and it builds on GCC, MSVC and Clang.

AcademySoftwareFoundation / OpenShadingLanguage