Zstd: Compiler macro warnings with -pedantic option

[MarcusKlik]

Thanks for a fantastic library!

When compiling with the -pedantic option a lot of ISO C99 warnings are generated, an example of which is

d:/Compiler/gcc-4.9.3/mingw_32/bin/gcc  -I"D:/RCompile/recent/R/include" -DNDEBUG -I. -Ifstcore -Ifstcore_v1 -Ifstcore/LZ4 -Ifstcore/ZSTD -Ifstcore/ZSTD/common -Ifstcore/ZSTD/decompress -Ifstcore/ZSTD/compress -I'D:/RCompile/CRANpkg/lib/4.0/Rcpp/include'   -I"d:/Compiler/gcc-4.9.3/local330/include"     -pedantic -O2 -Wall  -std=gnu99 -mtune=core2 -c fstcore/ZSTD/decompress/zstd_decompress.c -o fstcore/ZSTD/decompress/zstd_decompress.o
fstcore/ZSTD/decompress/zstd_decompress.c: In function 'ZSTD_frameHeaderSize_internal':
fstcore/ZSTD/decompress/zstd_decompress.c:211:58: warning: ISO C99 requires rest arguments to be used
     RETURN_ERROR_IF(srcSize < minInputSize, srcSize_wrong);

These warnings are due to macro's defined with ellipses that are left empty when called, like the macro's defined in this code.

Currently, R packages submitted to CRAN that include ZSTD as part of their source code fail the acceptance pre-tests because of these warnings. Would it be possible to include the -pedantic option in your gcc builds to help fix them?

thanks and all the best

[bimbashrestha]

I think this is a duplicate issue: https://github.com/facebook/zstd/issues/1538

[MarcusKlik]

Hi @bimbashrestha, thanks for your reply.

Yes, both issues are caused by calls to macro's defined with ellipsis arguments that are left unused.
Possible solutions are suggested here.

One of the suggestion in that post is to include the argument before the ellipsis in the ellipsis, that way the 'rest argument' is always used (by at least one parameter).

[bimbashrestha]

@MarcusKlik we had another contributor try and remedy this problem but I think at the time, we realized that the fix would be rather invasive. So ultimately, we decided that making zstd work with -pedantic was not worth the trade off in added complexity. See this comment for more info:

https://github.com/facebook/zstd/pull/1989#issuecomment-591189146

[MarcusKlik]

Hi @bimbashrestha, thanks for the pointer to @Cyan4973's comments on a similar issue. I agree fully that most solutions seem far to heavy for a few warnings generated with the -pedantic option!

It's just that the CRAN team (which reviews all R packages) pre-tests all submitted packages using this option, so any package that includes the ZSTD sources instead of using the native libraries will get some hits there :-)

That review process also includes clang-10 ubsan/asan tests and those found a few minor issues as well. I posted them below for completeness. They all boil down to ZSTD's use of BYTE* to calculate offsets in the byte stream (such as in this code). When that pointer equals a null pointer, the sanitizer checks fail (the reason is explained here). An easy fix is to use (uintptr_t) instead of BYTE* to calculate the offsets.

Thanks for reviewing my issue, feel free to close it at your discretion!

UBSAN/ASAN errors on clang10 (on ZSTD's latest release codebase):

fst.Rcheck/tests/testthat.Rout:fstcore/ZSTD/compress/zstd_compress.c:1323:46:

runtime error: applying non-zero offset 1 to null pointer

fst.Rcheck/tests/testthat.Rout:fstcore/ZSTD/compress/zstd_compress_internal.h:876:41:

runtime error: applying non-zero offset 1 to null pointer

fst.Rcheck/tests/testthat.Rout:fstcore/ZSTD/compress/zstd_compress_internal.h:877:33:

runtime error: applying non-zero offset 1 to null pointer

fst.Rcheck/tests/testthat.Rout:fstcore/ZSTD/compress/zstd_double_fast.c:99:62:

runtime error: applying zero offset to null pointer

fst.Rcheck/tests/testthat.Rout:fstcore/ZSTD/compress/zstd_opt.c:420:42:

runtime error: applying non-zero offset 1 to null pointer

fst.Rcheck/tests/testthat.Rout:fstcore/ZSTD/compress/zstd_opt.c:568:42:

runtime error: applying non-zero offset 1 to null pointer

fst.Rcheck/tests/testthat.Rout:fstcore/ZSTD/compress/zstd_opt.c:612:55:

runtime error: applying non-zero offset 4294967294 to null pointer

fst.Rcheck/tests/testthat.Rout:fstcore/ZSTD/compress/zstd_lazy.c:663:61:

runtime error: applying zero offset to null pointer

fst.Rcheck/tests/testthat.Rout:fstcore/ZSTD/compress/zstd_lazy.c:495:42:

runtime error: applying non-zero offset 1 to null pointer

fst.Rcheck/fst-Ex.Rout:fstcore/LZ4/lz4.c:818:44: runtime error: applying

zero offset to null pointer

fst.Rcheck/fst-Ex.Rout:fstcore/LZ4/lz4.c:828:40: runtime error: applying

zero offset to null pointer

fst.Rcheck/fst-Ex.Rout:fstcore/LZ4/lz4.c:828:51: runtime error: applying

zero offset to null pointer

fst.Rcheck/fst-Ex.Rout:fstcore/ZSTD/compress/zstd_compress.c:1323:46:

runtime error: applying non-zero offset 1 to null pointer

fst.Rcheck/fst-Ex.Rout:fstcore/ZSTD/compress/zstd_compress_internal.h:876:41:

runtime error: applying non-zero offset 1 to null pointer

fst.Rcheck/fst-Ex.Rout:fstcore/ZSTD/compress/zstd_compress_internal.h:877:33:

runtime error: applying non-zero offset 1 to null pointer

fst.Rcheck/fst-Ex.Rout:fstcore/ZSTD/compress/zstd_opt.c:420:42: runtime

error: applying non-zero offset 1 to null pointer

fst.Rcheck/fst-Ex.Rout:fstcore/ZSTD/compress/zstd_opt.c:568:42: runtime

error: applying non-zero offset 1 to null pointer

fst.Rcheck/fst-Ex.Rout:fstcore/ZSTD/compress/zstd_opt.c:612:55: runtime

error: applying non-zero offset 8445 to null pointer

[bimbashrestha]

@MarcusKlik thanks for providing the list of asan/ubsan failures on clang-10. Looks like there are quite a number of them and I don't know if we'll have time to handle these anytime soon.

Still this is good to make note of and we can flag it as something that might be worth fixing in the future. If anyone wants to put up a PR to handle these warnings, I'll be happy to review:)

[MarkCallow]

far to heavy for a few warnings

I get 110 warnings when compiling the single file decoder. That is way more than "a few".

And what if someone compiles this on an iso c99 compiler that really does require a least 1 argument hence the macro doesn't work?

[felixhandte]

2099 should have fixed the -pedantic warnings. So nominally, this issue is resolved. But there's also this conversation about the ubsan issues. Do you want to open a new issue for those? It seems distinct.

[MarcusKlik]

Hi @felixhandte, that's great, thanks for fixing the -pedantic warnings!

About the ubsan errors, as the current implementation might lead to undefined behavior (as explained here, I think it's important enough to open a separate issue (please see #2110).

Thanks again!