Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update pl-inline.h: end of data for murmur_key (please check carefully) #1341

Closed
wants to merge 2 commits into from

Conversation

mgondan
Copy link
Contributor

@mgondan mgondan commented Jan 3, 2025

With

CC="gcc-14 -std=gnu99 -fsanitize=address,undefined,bounds-strict -fno-omit-frame-pointer" CXX="g++ -std=gnu++17" CFLAGS="-g -O2 " CXXFLAGS="-g -O2  -fpic" cmake -DCMAKE_CXX_STANDARD=17 -S ..

I get a warning:

In function ‘murmur_key’,
    inlined from ‘indexKeyFromClause’ at /home/matthias/swipl-devel/src/pl-index.c:1455:12,
    inlined from ‘deleteActiveClauseFromIndex’ at /home/matthias/swipl-devel/src/pl-index.c:1475:14,
    inlined from ‘deleteActiveClauseFromIndexes’ at /home/matthias/swipl-devel/src/pl-index.c:1540:4:
/home/matthias/swipl-devel/src/pl-inline.h:765:31: warning: array subscript ‘word {aka long unsigned int}[3]’ is partly outside array bounds of ‘word[4]’ {aka ‘long unsigned int[4]’} [-Warray-bounds=]
  765 |     data[KEY_INDEX_MAX-2] = in[n/sizeof(word)-1]; // was n/sizeof(word)-1
      |                             ~~^~~~~~~~~~~~~~~~~~
/home/matthias/swipl-devel/src/pl-index.c: In function ‘deleteActiveClauseFromIndexes’:
/home/matthias/swipl-devel/src/pl-index.c:1441:10: note: at offset [25, 32] into object ‘key’ of size 32
 1441 |   { word key[MAX_MULTI_INDEX];                  /* TBD: special case for 1 arg */
      |          ^~~

Changing from -1 to -2 fixes the problem (and all tests in ctest pass). Needless to say, I have no idea what I am doing here :-P

With 

````
CC="gcc-14 -std=gnu99 -fsanitize=address,undefined,bounds-strict -fno-omit-frame-pointer" CXX="g++ -std=gnu++17" CFLAGS="-g -O2 " CXXFLAGS="-g -O2  -fpic" cmake -DCMAKE_CXX_STANDARD=17 -S ..
````

I get a warning:

````
In function ‘murmur_key’,
    inlined from ‘indexKeyFromClause’ at /home/matthias/swipl-devel/src/pl-index.c:1455:12,
    inlined from ‘deleteActiveClauseFromIndex’ at /home/matthias/swipl-devel/src/pl-index.c:1475:14,
    inlined from ‘deleteActiveClauseFromIndexes’ at /home/matthias/swipl-devel/src/pl-index.c:1540:4:
/home/matthias/swipl-devel/src/pl-inline.h:765:31: warning: array subscript ‘word {aka long unsigned int}[3]’ is partly outside array bounds of ‘word[4]’ {aka ‘long unsigned int[4]’} [-Warray-bounds=]
  765 |     data[KEY_INDEX_MAX-2] = in[n/sizeof(word)-1]; // was n/sizeof(word)-1
      |                             ~~^~~~~~~~~~~~~~~~~~
/home/matthias/swipl-devel/src/pl-index.c: In function ‘deleteActiveClauseFromIndexes’:
/home/matthias/swipl-devel/src/pl-index.c:1441:10: note: at offset [25, 32] into object ‘key’ of size 32
 1441 |   { word key[MAX_MULTI_INDEX];                  /* TBD: special case for 1 arg */
      |          ^~~
````

Changing from -1 to -2 fixes the problem (and all tests in ctest pass). Needless to say, I have no idea what I am doing here :-P
That change silences the warning too.

So either the array-bounds warning is a false positive, or the assertion n%sizeof(word) isn't always met.
@mgondan
Copy link
Contributor Author

mgondan commented Jan 4, 2025

Less intrusive now, but still: either the array-bounds warning is a false positive, or the assertion n%sizeof(word) isn't always met.

@JanWielemaker
Copy link
Member

I just rewrote that function in a private branch where I'm working on cleaning the indexing code (and eventually improving it). I'll see whether I can reproduce this and whether my changes already fix this (and re-read the code).

@mgondan
Copy link
Contributor Author

mgondan commented Jan 4, 2025

Great, thank you

@JanWielemaker
Copy link
Member

Ok, Patch 7b25563 supersedes this. There are quit a few runtime errors. Didn't look into them. Did you plan to have a look?

@mgondan
Copy link
Contributor Author

mgondan commented Jan 4, 2025

Yes, the runtime errors are next on my list, but they are unrelated. Thank you!

@JanWielemaker
Copy link
Member

Yes, the runtime errors are next on my list, but they are unrelated. Thank you!

I could not resist ... Fixed most with cfb2b8f, relying on new c99 behaviour (SWI-Prolog needs C11 anyway).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants