ATA_LOG_PAGE_LEN_BYTES defined using UINT16_C, then later used as UINT16_C(ATA_LOG_PAGE_LEN_BYTES)

[drboone]

Line 1912 of subprojects/opensea-operations/src/drive_info.c uses the construct UINT16_C(ATA_LOG_PAGE_LEN_BYTES), but the definition of that constant in ata_helper.h is #define ATA_LOG_PAGE_LEN_BYTES UINT16_C(512). At least on my SmartOS (Illumos distro) system with gcc 10.4, that results in the 'u' suffix getting appended to the value twice, and the following error:

lesmiz 61 $ ninja -C builddir
ninja: Entering directory `builddir'
[1/19] Compiling C object subprojects/opensea-operations/libopensea-operations.a.p/src_drive_info.c.o
FAILED: subprojects/opensea-operations/libopensea-operations.a.p/src_drive_info.c.o 
cc -Isubprojects/opensea-operations/libopensea-operations.a.p -Isubprojects/opensea-operations -I../subprojects/opensea-operations -I../subprojects/opensea-operations/include -I../subprojects/opensea-common/include -I../subprojects/opensea-transport/include -I../subprojects/opensea-transport/include/vendor -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -std=gnu99 -O3 -Wshadow=compatible-local -Wvla -Wfloat-equal -Wnull-dereference -Wunused-const-variable -Wunused-parameter -Wunused-value -Wduplicated-cond -Wjump-misses-init -Wstringop-overflow -Wlogical-op -Wshift-overflow -Wshift-overflow=1 -Wshift-overflow=2 -Wdouble-promotion -Wformat-security -Wold-style-definition -Wstrict-prototypes -Wmissing-declarations -Wmissing-prototypes -Wchar-subscripts -Wundef -Wformat -Wformat=2 -Wint-conversion -Wenum-conversion -Wfloat-conversion -Wint-to-pointer-cast -Wimplicit-fallthrough -D_GLIBCXX_ASSERTIONS -fstack-protector-strong -fno-delete-null-pointer-checks -fno-strict-overflow -fno-strict-aliasing -Wtrampolines -Werror=implicit -Werror=incompatible-pointer-types -Werror=int-conversion -Werror=implicit-int -Woverlength-strings -Wparentheses -Wcast-qual -Wuninitialized -Wvarargs -Wwrite-strings -Wrestrict -Wstringop-truncation -Werror=trigraphs -Wunreachable-code -Wcomment -Wsequence-point -Wreturn-type -fvisibility=hidden -Wsign-conversion -fstack-clash-protection -fcf-protection=full -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -ffunction-sections -fdata-sections -fPIC -DHAVE_MEMSET_S -DHAVE_POSIX_STRERR_R -MD -MQ subprojects/opensea-operations/libopensea-operations.a.p/src_drive_info.c.o -MF subprojects/opensea-operations/libopensea-operations.a.p/src_drive_info.c.o.d -o subprojects/opensea-operations/libopensea-operations.a.p/src_drive_info.c.o -c ../subprojects/opensea-operations/src/drive_info.c
In file included from /usr/include/sys/stdint.h:38,
                 from /usr/include/stdint.h:36,
                 from /opt/local/gcc10/lib/gcc/x86_64-sun-solaris2.11/10.4.0/include/stdint.h:9,
                 from ../subprojects/opensea-common/include/common_types.h:55,
                 from ../subprojects/opensea-operations/src/drive_info.c:14:
../subprojects/opensea-operations/src/drive_info.c: In function 'get_ATA_Drive_Info_From_ID_Data_Log':
../subprojects/opensea-transport/include/ata_helper.h:906:45: error: invalid suffix "uu" on integer constant
  906 |     #define ATA_LOG_PAGE_LEN_BYTES UINT16_C(512) //each page of a log is 512 bytes. A given log may be multiple pages long, or multiples of this value.
      |                                             ^~~
../subprojects/opensea-operations/src/drive_info.c:1912:147: note: in expansion of macro 'ATA_LOG_PAGE_LEN_BYTES'
 1912 |         for (uint16_t iter = ATA_ID_DATA_SUP_PG_LIST_OFFSET; iter < C_CAST(uint16_t, listLen + ATA_ID_DATA_SUP_PG_LIST_OFFSET) && iter < UINT16_C(ATA_LOG_PAGE_LEN_BYTES); ++iter)
      |                                                                                                                                                   ^~~~~~~~~~~~~~~~~~~~~~
ninja: build stopped: subcommand failed.

Simply removing the UINT16_C wrapper gets rid of the compile error.

Compiler:

lesmiz 59 $ cc -v
Using built-in specs.
COLLECT_GCC=/opt/local/gcc10/bin/cc
COLLECT_LTO_WRAPPER=/opt/local/gcc10/libexec/gcc/x86_64-sun-solaris2.11/10.4.0/lto-wrapper
Target: x86_64-sun-solaris2.11
Configured with: ../gcc-1f8c6b1d90dd69d36e9de7c1962b42e349561085/configure --with-local-prefix=/opt/local --enable-languages=c,c++,fortran,go,objc,lto --enable-__cxa_atexit --enable-initfini-array --disable-nls --disable-libitm --with-gnu-as --with-as=/opt/local/bin/gas --without-gnu-ld --with-ld=/bin/ld --with-zstd=no --prefix=/opt/local/gcc10 --build=x86_64-sun-solaris2.11 --host=x86_64-sun-solaris2.11 --infodir=/opt/local/gcc10/info --mandir=/opt/local/gcc10/man
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 10.4.0 (GCC) 

[vonericsen]

I know we have had another Illumos distro user report some differences from their GCC compiler in the past that did not show up on Linux or FreeBSD machines I have access to.
I think that is why the UINT16_C() doesn't do this on these other machines I have setup. In Linux, FreeBSD, and Windows that macro does not append any trailing values which is why this did not show up.

[drboone]

Digging around in the system headers, the definition is:

/* CSTYLED */
#define UINT16_C(c) __CONCAT__(c,u)

and there's a Sun comment about the CSTYLED thing being a flag for an internal code style auditing tool. That would seem to explain its uniqueness.

Thanks for your fast responses!

[vonericsen]

@drboone,

No problem. These are all pretty straight forward things to correct.
We always try to keep all platforms building so that the tools are easier to get when they are needed.

I'm exploring other tools we can use to help identify these kinds of mistakes to prevent this in the future. CI would be best but I'm currently playing with clang-tidy to help find issues as well...IDK if it would have prevented these issues, but I'll add whatever I can to help prevent this from being an issue in the future.

[drboone]

Jonathan Perkin from the SmartOS team, who wrangles pkgsrc, said:

there are vmactions images for omnios, so folks can use github actions to kick off builds, though it's a little involved

I set up an entirely github actions driven CI thing for pkgsrc that tested on a bunch of different platforms with not a lot of work, only problem is it burns through the free tier pretty quick

but e.g. https://github.com/pkgsrc-ci/pkgsrc/actions/runs/10058351362 tests a commit across a whole bunch of OS (including cygwin ;)

[drboone]

Er. Put this in the wrong ticket, because I kin reed, I sware...

[vonericsen]

@drboone,

I will take a look at it! Thank you for asking that and sharing the link!