gh-Cyan4973/xxHash

Watch

Fork 0

GH-Actions (todo) : Add `-Og` and `gcc-12` #810

New issue

Closed

opened 2023-03-02 18:42:09 -06:00 by t-mat · 3 comments

t-mat commented

2023-03-02 18:42:09 -06:00

(Migrated from github.com)

- https://github.com/Cyan4973/xxHash/pull/804#issuecomment-1447244510 - https://github.com/Cyan4973/xxHash/issues/800

👍 1

t-mat commented

2023-03-05 00:06:32 -06:00

(Migrated from github.com)

v0.8.1 and HEAD may have issue which is not related to inline problems. But -Og helps us to detect it.

git clone https://github.com/Cyan4973/xxHash.git xxHash-v0.8.1 --branch v0.8.1 --single-branch --depth 1
cd xxHash-v0.8.1

make clean all
# OK

MOREFLAGS="-Og" make clean all
# NG (warnings)

Here's actual warning messages.

cc -O3 -Wall -Wextra -Wconversion -Wcast-qual -Wcast-align -Wshadow -Wstrict-aliasing=1 -Wswitch-enum -Wdeclaration-after-statement -Wstrict-prototypes -Wundef -Wpointer-arith -Wformat-security -Wvla -Wformat=2 -Winit-self -Wfloat-equal -Wwrite-strings -Wredundant-decls -Wstrict-overflow=2 -Og -DXXH_INLINE_ALL cli/xxhsum.c -o xxhsum_inlinedXXH
In file included from cli/xsum_sanity_check.c:31,
                 from cli/xxhsum.c:41:
In function ‘XXH_INLINE_XXH3_64bits_reset_withSeed’,
    inlined from ‘XXH_INLINE_XXH3_128bits_reset_withSeed’ at cli/../xxhash.h:5402:12,
    inlined from ‘localXXH128_stream_seeded’ at cli/xsum_bench.c:176:5:
cli/../xxhash.h:4733:46: warning: ‘state.extSecret’ may be used uninitialized [-Wmaybe-uninitialized]
 4733 |     if ((seed != statePtr->seed) || (statePtr->extSecret != NULL))
      |                                      ~~~~~~~~^~~~~~~~~~~
In file included from cli/xxhsum.c:42:
cli/xsum_bench.c: In function ‘localXXH128_stream_seeded’:
cli/xsum_bench.c:174:18: note: ‘state’ declared here
  174 |     XXH3_state_t state;
      |                  ^~~~~
In file included from cli/xsum_sanity_check.c:31,
                 from cli/xxhsum.c:41:
In function ‘XXH_INLINE_XXH3_64bits_reset_withSeed’,
    inlined from ‘localXXH3_stream_seeded’ at cli/xsum_bench.c:160:5:
cli/../xxhash.h:4733:46: warning: ‘state.extSecret’ may be used uninitialized [-Wmaybe-uninitialized]
 4733 |     if ((seed != statePtr->seed) || (statePtr->extSecret != NULL))
      |                                      ~~~~~~~~^~~~~~~~~~~
In file included from cli/xxhsum.c:42:
cli/xsum_bench.c: In function ‘localXXH3_stream_seeded’:
cli/xsum_bench.c:158:18: note: ‘state’ declared here
  158 |     XXH3_state_t state;
      |                  ^~~~~

It warns that at xxhash.h#L4733, if (statePtr != NULL) && (seed != 0) && !(seed != seedPtr->seed), we may read uninitialized variable statePtr->extSecret.

https://github.com/Cyan4973/xxHash/blob/v0.8.1/xxhash.h#L4733

        XXH_PUBLIC_API XXH_errorcode
        XXH3_64bits_reset_withSeed(XXH3_state_t* statePtr, XXH64_hash_t seed)
        {
            if (statePtr == NULL) return XXH_ERROR;
            if (seed==0) return XXH3_64bits_reset(statePtr);
/*4733*/    if ((seed != statePtr->seed) || (statePtr->extSecret != NULL))
                XXH3_initCustomSecret(statePtr->customSecret, seed);
            XXH3_reset_internal(statePtr, seed, NULL, XXH_SECRET_DEFAULT_SIZE);
            return XXH_OK;
        }

Our initizliation is done by XXH3_INITSTATE(). Note that we don't have any explicit initializer list for state.

https://github.com/Cyan4973/xxHash/blob/v0.8.1/cli/xsum_bench.c#L158

        static XSUM_U32 localXXH3_stream_seeded(const void* buffer, size_t bufferSize, XSUM_U32 seed)
        {
/*158*/     XXH3_state_t state;
            XXH3_INITSTATE(&state);
            XXH3_64bits_reset_withSeed(&state, (XXH64_hash_t)seed);
            XXH3_64bits_update(&state, buffer, bufferSize);
            return (XSUM_U32)XXH3_64bits_digest(&state);
        }

XXH3_INITSTATE() only set statePtr->seed. But it doesn't touch statePtr->extSecret.

https://github.com/Cyan4973/xxHash/blob/v0.8.1/xxhash.h#L1113

#define XXH3_INITSTATE(XXH3_state_ptr)   { (XXH3_state_ptr)->seed = 0; }

I think adding (XXH3_state_ptr)->extSecret = NULL; to XXH3_INITSTATE() is the most easy and safe fix for users who doesn't care about internals of XXH3.
But not sure about benchmarks.

`v0.8.1` and [`HEAD`](https://github.com/Cyan4973/xxHash/tree/2b328a10983d232364ceda15df1d33531b5f0eb5) may have issue which is not related to `inline` problems. But `-Og` helps us to detect it. ``` git clone https://github.com/Cyan4973/xxHash.git xxHash-v0.8.1 --branch v0.8.1 --single-branch --depth 1 cd xxHash-v0.8.1 make clean all # OK MOREFLAGS="-Og" make clean all # NG (warnings) ``` <details><summary>Here's actual warning messages.</summary> ``` cc -O3 -Wall -Wextra -Wconversion -Wcast-qual -Wcast-align -Wshadow -Wstrict-aliasing=1 -Wswitch-enum -Wdeclaration-after-statement -Wstrict-prototypes -Wundef -Wpointer-arith -Wformat-security -Wvla -Wformat=2 -Winit-self -Wfloat-equal -Wwrite-strings -Wredundant-decls -Wstrict-overflow=2 -Og -DXXH_INLINE_ALL cli/xxhsum.c -o xxhsum_inlinedXXH In file included from cli/xsum_sanity_check.c:31, from cli/xxhsum.c:41: In function ‘XXH_INLINE_XXH3_64bits_reset_withSeed’, inlined from ‘XXH_INLINE_XXH3_128bits_reset_withSeed’ at cli/../xxhash.h:5402:12, inlined from ‘localXXH128_stream_seeded’ at cli/xsum_bench.c:176:5: cli/../xxhash.h:4733:46: warning: ‘state.extSecret’ may be used uninitialized [-Wmaybe-uninitialized] 4733 | if ((seed != statePtr->seed) || (statePtr->extSecret != NULL)) | ~~~~~~~~^~~~~~~~~~~ In file included from cli/xxhsum.c:42: cli/xsum_bench.c: In function ‘localXXH128_stream_seeded’: cli/xsum_bench.c:174:18: note: ‘state’ declared here 174 | XXH3_state_t state; | ^~~~~ In file included from cli/xsum_sanity_check.c:31, from cli/xxhsum.c:41: In function ‘XXH_INLINE_XXH3_64bits_reset_withSeed’, inlined from ‘localXXH3_stream_seeded’ at cli/xsum_bench.c:160:5: cli/../xxhash.h:4733:46: warning: ‘state.extSecret’ may be used uninitialized [-Wmaybe-uninitialized] 4733 | if ((seed != statePtr->seed) || (statePtr->extSecret != NULL)) | ~~~~~~~~^~~~~~~~~~~ In file included from cli/xxhsum.c:42: cli/xsum_bench.c: In function ‘localXXH3_stream_seeded’: cli/xsum_bench.c:158:18: note: ‘state’ declared here 158 | XXH3_state_t state; | ^~~~~ ``` </details> It warns that at xxhash.h#L4733, if `(statePtr != NULL) && (seed != 0) && !(seed != seedPtr->seed)`, we may read uninitialized variable `statePtr->extSecret`. https://github.com/Cyan4973/xxHash/blob/v0.8.1/xxhash.h#L4733 ```C XXH_PUBLIC_API XXH_errorcode XXH3_64bits_reset_withSeed(XXH3_state_t* statePtr, XXH64_hash_t seed) { if (statePtr == NULL) return XXH_ERROR; if (seed==0) return XXH3_64bits_reset(statePtr); /*4733*/ if ((seed != statePtr->seed) || (statePtr->extSecret != NULL)) XXH3_initCustomSecret(statePtr->customSecret, seed); XXH3_reset_internal(statePtr, seed, NULL, XXH_SECRET_DEFAULT_SIZE); return XXH_OK; } ``` Our initizliation is done by `XXH3_INITSTATE()`. Note that we don't have any explicit initializer list for `state`. https://github.com/Cyan4973/xxHash/blob/v0.8.1/cli/xsum_bench.c#L158 ```C static XSUM_U32 localXXH3_stream_seeded(const void* buffer, size_t bufferSize, XSUM_U32 seed) { /*158*/ XXH3_state_t state; XXH3_INITSTATE(&state); XXH3_64bits_reset_withSeed(&state, (XXH64_hash_t)seed); XXH3_64bits_update(&state, buffer, bufferSize); return (XSUM_U32)XXH3_64bits_digest(&state); } ``` `XXH3_INITSTATE()` only set `statePtr->seed`. But it doesn't touch `statePtr->extSecret`. https://github.com/Cyan4973/xxHash/blob/v0.8.1/xxhash.h#L1113 ``` #define XXH3_INITSTATE(XXH3_state_ptr) { (XXH3_state_ptr)->seed = 0; } ``` I think adding `(XXH3_state_ptr)->extSecret = NULL;` to `XXH3_INITSTATE()` is the most easy and safe fix for users who doesn't care about internals of XXH3. But not sure about benchmarks.

t-mat commented

2023-03-05 01:45:30 -06:00

(Migrated from github.com)

Since this is a false positive, splitting the condition may help compiler to understand the code.
It's ugly, and not sure about performance/codegen and maintainability.

XXH3_64bits_reset_withSeed(XXH_NOESCAPE XXH3_state_t* statePtr, XXH64_hash_t seed)
{
    ...
-   if ((seed != statePtr->seed) || (statePtr->extSecret != NULL))
-       XXH3_initCustomSecret(statePtr->customSecret, seed);

+   if ((seed != statePtr->seed))
+       XXH3_initCustomSecret(statePtr->customSecret, seed);
+   if ((statePtr->extSecret != NULL))
+       XXH3_initCustomSecret(statePtr->customSecret, seed);
    ...
}

Since this is a false positive, splitting the condition may help compiler to understand the code. It's ugly, and not sure about performance/codegen and maintainability. ```C XXH3_64bits_reset_withSeed(XXH_NOESCAPE XXH3_state_t* statePtr, XXH64_hash_t seed) { ... - if ((seed != statePtr->seed) || (statePtr->extSecret != NULL)) - XXH3_initCustomSecret(statePtr->customSecret, seed); + if ((seed != statePtr->seed)) + XXH3_initCustomSecret(statePtr->customSecret, seed); + if ((statePtr->extSecret != NULL)) + XXH3_initCustomSecret(statePtr->customSecret, seed); ... } ```

easyaspi314 commented

2023-03-05 17:38:14 -06:00

(Migrated from github.com)

I think adding (XXH3_state_ptr)->extSecret = NULL; to XXH3_INITSTATE() is the most easy and safe fix for users who doesn't care about internals of XXH3.
But I'm not sure about benchmarks.

I assure you that a single store is going to have no effect on performance.

However we should avoid double evaluation and use a do-while to make it a single statement (to avoid the if (x) MACRO; issue)

#define XXH3_INITSTATE(XXH3_state_ptr) \
    do { \
        XXH3_state_t *_state_ptr = (XXH3_state_ptr); \
        _state_ptr->seed = 0; \
        _state_ptr->extSecret = NULL; \
    } while (0)

> I think adding (XXH3_state_ptr)->extSecret = NULL; to XXH3_INITSTATE() is the most easy and safe fix for users who doesn't care about internals of XXH3. > But I'm not sure about benchmarks. I assure you that a single store is going to have no effect on performance. However we should avoid double evaluation and use a do-while to make it a single statement (to avoid the `if (x) MACRO;` issue) ```c #define XXH3_INITSTATE(XXH3_state_ptr) \ do { \ XXH3_state_t *_state_ptr = (XXH3_state_ptr); \ _state_ptr->seed = 0; \ _state_ptr->extSecret = NULL; \ } while (0) ```

👍 2

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-Cyan4973/xxHash#810

No description provided.

Rows
Columns

GH-Actions (todo) : Add -Og and gcc-12 #810

GH-Actions (todo) : Add `-Og` and `gcc-12` #810