dvidelabs / flatcc

FlatBuffers Compiler and Library in C for C
Apache License 2.0
631 stars 180 forks source link

presumed issue when compiling with LTO + GCC #238

Closed jobol closed 3 months ago

jobol commented 2 years ago

When compiling with GCC 11.2.0 (ubuntu 22) with link time optimizations the error below is reported

 In function 'ht_default_hash_function',
     inlined from 'ptr_set_hash_function' at ../compiler/./external/hash/ptr_set.c:58:12,
     inlined from 'ht_insert.constprop' at ../compiler/./external/hash/hash_table_impl.h:125:9: 
       ../compiler/./external/hash/ht_hash_function.h:24:5: warning: 's' may be used uninitialized [-Wmaybe-uninitialized]
       ../compiler/./external/hash/hash_table_impl.h: In function 'ht_insert.constprop':
       ../compiler/./external/hash/cmetrohash64.c:29:6: note: by argument 1 of type 'const uint8_t *' to 'cmetrohash64_1.constprop' declared here 
      ../compiler/./external/hash/hash_table_impl.h:106:18: note: 's' declared here

Also when using flatcc generated (even after the error report) with the following command line:

$ flatcc --json-printer refection/refection.fbs

a strange error is reported most of the time:

reflection.fbs:34:15: error: 'BaseTyp': unknown type reference used with table field: reflection.fbs:34:5: 'base_type'
reflection.fbs:35:13: error: 'BaseTyp': unknown type reference used with table field: reflection.fbs:35:5: 'element'
reflection.fbs:51:12: error: 'Objec': unknown type reference used with table field: reflection.fbs:51:5: 'object'
reflection.fbs:52:16: error: 'Typ': unknown type reference used with table field: reflection.fbs:52:5: 'union_type'
reflection.fbs:58:13: error: 'EnumVa': unknown vector type reference used with table field: reflection.fbs:58:5: 'values'
reflection.fbs:60:21: error: 'Typ': unknown type reference used with table field: reflection.fbs:60:5: 'underlying_type'
reflection.fbs:61:17: error: 'KeyValu': unknown vector type reference used with table field: reflection.fbs:61:5: 'attributes'
reflection.fbs:67:10: error: 'Typ': unknown type reference used with table field: reflection.fbs:67:5: 'type'
reflection.fbs:75:17: error: 'KeyValu': unknown vector type reference used with table field: reflection.fbs:75:5: 'attributes'
reflection.fbs:82:13: error: 'Fiel': unknown vector type reference used with table field: reflection.fbs:82:5: 'fields'
reflection.fbs:86:17: error: 'KeyValu': unknown vector type reference used with table field: reflection.fbs:86:5: 'attributes'
reflection.fbs:92:13: error: 'Objec': unknown type reference used with table field: reflection.fbs:92:5: 'request'
reflection.fbs:93:14: error: 'Objec': unknown type reference used with table field: reflection.fbs:93:5: 'response'
reflection.fbs:94:17: error: 'KeyValu': unknown vector type reference used with table field: reflection.fbs:94:5: 'attributes'
reflection.fbs:100:12: error: 'RPCCal': unknown vector type reference used with table field: reflection.fbs:100:5: 'calls'
reflection.fbs:101:17: error: 'KeyValu': unknown vector type reference used with table field: reflection.fbs:101:5: 'attributes'
reflection.fbs:106:14: error: 'Objec': unknown vector type reference used with table field: reflection.fbs:106:5: 'objects'
reflection.fbs:107:12: error: 'Enu': unknown vector type reference used with table field: reflection.fbs:107:5: 'enums'
reflection.fbs:110:16: error: 'Objec': unknown type reference used with table field: reflection.fbs:110:5: 'root_table'
reflection.fbs:111:15: error: 'Servic': unknown vector type reference used with table field: reflection.fbs:111:5: 'services'
reflection.fbs:114:11: error: 'Schem': root type not found

Investigations gave nothing on my side (for the time I can give). But removing LTO solves the issue.

mikkelfj commented 2 years ago

Metrohash has come up a few times recently. We may have to replace it - would you be up for testing xxhash as an alternative?

jobol commented 2 years ago

Do you mean "are you up to code integration of xxhash" or just "are you up for testing an already done draft"?

mikkelfj commented 2 years ago

I think I already have a xxhash impl. that fit the used interface. It would be good to know if you can see if the issue is in Metrohash, or in the interfacing code like ptrset and ht_hash_function etc.

Seeing your next message - I'd say both. I can look for a proper intergation to give you.

jobol commented 2 years ago

It is easy for me to check an existing proposal. But okay for both but at spare time.

mikkelfj commented 2 years ago

OK, I added a new branch "hash" with a new directory "external/hash2". I will delete it again later and it is not intended for direct unqualified integration. But it uses the same interface, possible at a different revision, and it includes MurMurhash and xxhash. The interface allows to create different hash table implementations based on a single hash function.

mikkelfj commented 2 years ago

FYI: the clang sanitizer also has a problem with metrohash, see log output of first message in issue: https://github.com/dvidelabs/flatcc/issues/236

flatcc/external/hash/cmetrohash.h:62:22: runtime error: load of misaligned address 0x562a0e5a99fe for type 'uint32_t' (aka 'unsigned int'), which requires 4 byte alignment
0x562a0e5a99fe: note: pointer points here
mikkelfj commented 8 months ago

Please check branch https://github.com/dvidelabs/flatcc/tree/hash if you have time. I updated the branch with latest on master and it should replace cmetrohash experimentally. I don't have a reproducible setup. If it works for you I can replace the hash function, but I don't know where the issue is exactly.

jobol commented 8 months ago

I'm focused on some other work at the moment but I could find some time next week to check that.

mikkelfj commented 8 months ago

Great, thanks.

mikkelfj commented 8 months ago

I forgot to enable xxhash on that branch. I have pushed an update. I would be interested in both warnings from cmetrohash on the main branch and how xxhash works on the hash branch. FYI: it is possible to enable cmetrohash again in the hash branch by defining the C flag HT_HASH_FALLBACK, and uncommenting a line removing cmetrohash in src/compiler/CMakeList.txt.

mikkelfj commented 8 months ago

@jobol this is the only issue left before a 0.6.2 release is ready, not to rush, just a heads up. I am inclined to a potential hash table change until after the release because it might break things somewhere, but it would still be good to know if there is something that ought to be fixed before the release.

EDIT: forgot about --outfile, that is also pending that you take a look.

mikkelfj commented 3 months ago

I merged xxhash changes to master. I don't know if it fixes the reported issue, but xxhash is the better hash table either way.

Closing as potentially fixed due to lack of feedback.