kuzudb / kuzu

Embeddable property graph database management system built for query speed and scalability. Implements Cypher.
https://kuzudb.com/
MIT License
1.38k stars 97 forks source link

Don't self-initialize schema comments #2038

Closed benjaminwinger closed 1 year ago

benjaminwinger commented 1 year ago

I was getting tons of segfaults on my computer when running tests. It turned out that the new comment field in the TableSchema is being initialized to itself in one of the constructors.

Is the comment supposed to be passed as an argument to this constructor? I just default-initialized it instead, which fixed the crashes.

Riolku commented 1 year ago

Yikes. Sorry about that, any reason why our tests don't catch this?

Is the comment supposed to be passed as an argument to this constructor?

Not this one. By default the comment is supposed to be empty, but the second constructor is used as a "more complete" one, for copying, AFAICT.

benjaminwinger commented 1 year ago

It was only happening in release mode, I had to compile with the RelWithDebInfo configuration to debug it (which turned a dozen or so crashes into every single database test crashing immediately). So it might vary with the compiler: I think CI is using gcc 11, while I'm building with gcc 13.

A quick test in godbolt shows that it produces a segfault on gcc 12+, and succeeds on gcc <11. It also works fine in clang, which is the only one to produce a warning.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% :warning:

Comparison is base (4514dd4) 90.14% compared to head (d60da3b) 90.14%. Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2038 +/- ## ========================================== - Coverage 90.14% 90.14% -0.01% ========================================== Files 934 938 +4 Lines 33365 33364 -1 ========================================== - Hits 30077 30076 -1 Misses 3288 3288 ``` | [Files Changed](https://app.codecov.io/gh/kuzudb/kuzu/pull/2038?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kuzudb) | Coverage Δ | | |---|---|---| | [src/include/catalog/table\_schema.h](https://app.codecov.io/gh/kuzudb/kuzu/pull/2038?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kuzudb#diff-c3JjL2luY2x1ZGUvY2F0YWxvZy90YWJsZV9zY2hlbWEuaA==) | `100.00% <100.00%> (ø)` | | ... and [43 files with indirect coverage changes](https://app.codecov.io/gh/kuzudb/kuzu/pull/2038/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kuzudb)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Riolku commented 1 year ago

Yet another reason to use clang :wink:

benjaminwinger commented 1 year ago

Except there are so many warnings in the codebase that you wouldn't notice with clang normally. If we could clean up all the warnings and turn them into errors, then it could have been caught by the clang CI.

Riolku commented 1 year ago

ANTLR4 is particularly loud. I might look into reducing our warnings, ive always liked Werror