sys.h: Fix __attribute__ usage for non-GCC compilers #884
Conversation
|
@gilles-peskine-arm, I hope that this solves your issues with MSVC and IAR. Could you try it out? Btw, feel free to tag @pq-code-package/pqcp-mldsa-native-admin for issues like this in the future. We'll try to help if we have some spare cycles. This time we only saw it coincidentally. |
mkannwischer
force-pushed
the
fix-mld-inline
branch
3 times, most recently
from
61db8f5 to
fb8b926
Compare
hanno-becker
left a comment
hanno-becker
left a comment
There was a problem hiding this comment.
Thanks @gilles-peskine-arm for noting this, and @mkannwischer for analyzing and fixing. LGTM.
hanno-becker
force-pushed
the
fix-mld-inline
branch
from
fb8b926 to
33cac9e
Compare
gilles-peskine-arm
left a comment
gilles-peskine-arm
left a comment
There was a problem hiding this comment.
Thank you very much! I can confirm that 33cac9e fixes the build¹ with IAR 9 (which TF-PSA-Crypto officially supports due to user demand), and also ARM Compiler 5 (which neither Arm not TF-PSA-Crypto officially supports any longer). I also tried other compilers I could find on my Ubuntu machine (and that we don't care about): tcc implements enough GCC extensions that it was happy before, bcc doesn't provide stdint.h², pcc's preprocessor seems to get confused and I don't care enough to see if that's fixable, and sdcc now compiles with warnings that suggest that the library won't work on a 16-bit platform³.
¹ I only tried ${CC} -c -I. -DMLD_CONFIG_PARAMETER_SET=87 mldsa_native.c with a toy definition of mld_zeroize in mldsa_native_config.h. I'll see if I can run tests when I've made progress with our integration.
² I think that means your claim of “portable C90” implementation in README.md is wrong: stdint.h was a popular extension to C90, but it only became standard in C99.
³ I'm not sure whether it's realistic that a platform with 16-bit int could run ML-DSA, but this should be documented as well.
For mlkem-native, we already have support and tests for C implementations with 16-bit |
|
@gilles-peskine-arm Thank you for all these references to compilers I have never heard of before 😄
Oh, ok, I was not aware of that. We should remark this. |
Restructure `MLD_INLINE` and `MLD_ALWAYS_INLINE` into separate blocks, each independently guarded: `MLD_INLINE`: - MSVC: `__inline` - C99 or `inline` macro defined: `inline` - GCC/Clang C90: `__attribute__((unused))` to silence warnings - Other C90: empty `MLD_ALWAYS_INLINE`: - MSVC: `__forceinline` - GCC/Clang C99+: `MLD_INLINE __attribute__((always_inline))` - Other: `MLD_INLINE` (no forced inlining) This fixes two issues: 1. If `inline` was defined as a macro before including sys.h, the MSVC check was bypassed and `__attribute__` was used, causing MSVC failures. 2. For non-MSVC/GCC/Clang compilers like IAR, `__attribute__` was used unconditionally, causing build failures. Also fix typo: `defined(clang)` -> `defined(__clang__)` Signed-off-by: Matthias J. Kannwischer <[email protected]>
hanno-becker
force-pushed
the
fix-mld-inline
branch
from
33cac9e to
6b2092d
Compare
Restructure `MLD_INLINE` and `MLD_ALWAYS_INLINE` into separate blocks, each independently guarded: `MLD_INLINE`: - MSVC: `__inline` - C99 or `inline` macro defined: `inline` - GCC/Clang C90: `__attribute__((unused))` to silence warnings - Other C90: empty `MLD_ALWAYS_INLINE`: - MSVC: `__forceinline` - GCC/Clang C99+: `MLD_INLINE __attribute__((always_inline))` - Other: `MLD_INLINE` (no forced inlining) This fixes two issues: 1. If `inline` was defined as a macro before including sys.h, the MSVC check was bypassed and `__attribute__` was used, causing MSVC failures. 2. For non-MSVC/GCC/Clang compilers like IAR, `__attribute__` was used unconditionally, causing build failures. Also fix typo: `defined(clang)` -> `defined(__clang__)` - Ported from pq-code-package/mldsa-native#884 Signed-off-by: Matthias J. Kannwischer <[email protected]>
Restructure `MLK_INLINE` and `MLK_ALWAYS_INLINE` into separate blocks, each independently guarded: `MLK_INLINE`: - MSVC: `__inline` - C99 or `inline` macro defined: `inline` - GCC/Clang C90: `__attribute__((unused))` to silence warnings - Other C90: empty `MLK_ALWAYS_INLINE`: - MSVC: `__forceinline` - GCC/Clang C99+: `MLK_INLINE __attribute__((always_inline))` - Other: `MLK_INLINE` (no forced inlining) This fixes two issues: 1. If `inline` was defined as a macro before including sys.h, the MSVC check was bypassed and `__attribute__` was used, causing MSVC failures. 2. For non-MSVC/GCC/Clang compilers like IAR, `__attribute__` was used unconditionally, causing build failures. Also fix typo: `defined(clang)` -> `defined(__clang__)` - Ported from pq-code-package/mldsa-native#884 Signed-off-by: Matthias J. Kannwischer <[email protected]>
Restructure `MLK_INLINE` and `MLK_ALWAYS_INLINE` into separate blocks, each independently guarded: `MLK_INLINE`: - MSVC: `__inline` - C99 or `inline` macro defined: `inline` - GCC/Clang C90: `__attribute__((unused))` to silence warnings - Other C90: empty `MLK_ALWAYS_INLINE`: - MSVC: `__forceinline` - GCC/Clang C99+: `MLK_INLINE __attribute__((always_inline))` - Other: `MLK_INLINE` (no forced inlining) This fixes two issues: 1. If `inline` was defined as a macro before including sys.h, the MSVC check was bypassed and `__attribute__` was used, causing MSVC failures. 2. For non-MSVC/GCC/Clang compilers like IAR, `__attribute__` was used unconditionally, causing build failures. Also fix typo: `defined(clang)` -> `defined(__clang__)` - Ported from pq-code-package/mldsa-native#884 Signed-off-by: Matthias J. Kannwischer <[email protected]>
Restructure
MLD_INLINEandMLD_ALWAYS_INLINEinto separate blocks,each independently guarded:
MLD_INLINE:__inlineinlinemacro defined:inline__attribute__((unused))to silence warningsMLD_ALWAYS_INLINE:__forceinlineMLD_INLINE __attribute__((always_inline))MLD_INLINE(no forced inlining)This fixes two issues:
inlinewas defined as a macro before including sys.h, the MSVCcheck was bypassed and
__attribute__was used, causing MSVC failures.__attribute__was usedunconditionally, causing build failures.
Also fix typo:
defined(clang)->defined(__clang__)