Bug #2077
String0::IsEqual causing warnings with newer GCC
| Status: | Approved | Start date: | 09/25/2020 | |
|---|---|---|---|---|
| Priority: | Normal | Due date: | ||
| Assignee: | Zbigniew Rebacz | % Done: | 0% | |
| Category: | Core | Spent time: | - | |
| Target version: | Release 2020.2 |
Description
Here is the warnings generated by gcc on Linux:
/home/klugier/upp/uppsrc/Core/AString.hpp: In member function ‘void Ide::ResolveUvsConflict()’:
/home/klugier/upp/uppsrc/Core/AString.hpp:269:36: warning: ‘int __builtin_memcmp_eq(const void*, const void*, long unsigned int)’ reading 17 bytes from a region of size 16 [-Wstringop-overflow=]
269 | return len == GetCount() && memcmp(begin(), s, len) == 0; // compiler is happy to optimize memcmp out...
| ~~~~~~^~~~~~~~~~~~~~~~~
/home/klugier/upp/uppsrc/Core/AString.hpp:269:36: warning: ‘int __builtin_memcmp_eq(const void*, const void*, long unsigned int)’ reading 17 bytes from a region of size 16 [-Wstringop-overflow=]
269 | return len == GetCount() && memcmp(begin(), s, len) == 0; // compiler is happy to optimize memcmp out...
| ~~~~~~^~~~~~~~~~~~~~~~~
/home/klugier/upp/uppsrc/Core/AString.hpp:269:36: warning: ‘int __builtin_memcmp_eq(const void*, const void*, long unsigned int)’ reading 21 bytes from a region of size 16 [-Wstringop-overflow=]
269 | return len == GetCount() && memcmp(begin(), s, len) == 0; // compiler is happy to optimize memcmp out...
|
Solution that fix the warning (not sure about performance):
inline
bool String0::IsEqual(const char *s) const
{ // This optimized for comparison with string literals...
size_t len = strlen(s);
return len == GetCount() && strncmp(begin(), s, len) == 0;
}
Tested on GCC 10.2.
History
#1 Updated by Miroslav Fidler over 3 years ago
- Status changed from New to Ready for QA
Posting GCC version would be really helpful.
Anyway, while perhaps negligible, the performance impact would be there. I believe this is basically GCC bug - he does not see the logic in begin()...
So we should find a way how to silence that... Maybe just replace with memeq8?
#2 Updated by Miroslav Fidler over 3 years ago
For record, no warnings with gcc 7.5.0
#3 Updated by Zbigniew Rebacz over 3 years ago
This is gcc 10.2
#4 Updated by Zbigniew Rebacz over 3 years ago
- Description updated (diff)
#5 Updated by Zbigniew Rebacz over 3 years ago
No warning with following line:
return len == GetCount() && memeq8(begin(), s, len) == 0; // compiler is happy to optimize memcmp out...
#6 Updated by Zbigniew Rebacz over 3 years ago
With memeq8 strange things happen inside TheIDE - files are not loaded correctly, so it is definitely no function we would like to use...
#7 Updated by Miroslav Fidler over 3 years ago
Without == 0!!!!
memeq returns true if equal...
#8 Updated by Zbigniew Rebacz over 3 years ago
Works fine without comparison and doesn't produce warning.
#9 Updated by Miroslav Fidler over 3 years ago
Unfortunately, the performance impact is still ugly. With memcmp, compiler can optimize constant comparison (like s == "test") into basically simple integer cmp...
What distro are you testing with? I will install in VirtualBox and find a way to silence that...
Mirek
#10 Updated by Zbigniew Rebacz over 3 years ago
I use Manjaro KDE edition. This is rolling distro, so you all the time should have decent compilers.
#11 Updated by Miroslav Fidler over 3 years ago
OK, I have installed manjaro.
gcc --version
10.2.0
With -Wall it gave warning in IsEqual0 about comparing size_t with int (ok, that is worth fixing), but not the warning you have posted.
Maybe you should update your gcc?
Mirek
#12 Updated by Miroslav Fidler over 3 years ago
- Status changed from Ready for QA to In Progress
- Assignee changed from Miroslav Fidler to Zbigniew Rebacz
#13 Updated by Miroslav Fidler over 3 years ago
Ah, ok, it is in release only...
#14 Updated by Zbigniew Rebacz over 3 years ago
Yes, I compile TheIDE in release mode. GCC already updated :)
#15 Updated by Miroslav Fidler over 3 years ago
- Status changed from In Progress to Ready for QA
I took me two days to figure all warnings out, but it should now compile with -Wall (except that && parenthesis issue).
The problem with String0::IsEqual is strongly GCC bug, but in the end I have figured out a fix that actually might improve the performance a bit...
#16 Updated by Zbigniew Rebacz over 3 years ago
- Status changed from Ready for QA to Approved
No warnings anymore - good job :)
#17 Updated by Zbigniew Rebacz over 3 years ago
- Status changed from Approved to New
- Assignee changed from Zbigniew Rebacz to Miroslav Fidler
Seems like the problem returned (tutorial/Skylark10 package), however in different place:
from /home/klugier/upp/uppsrc/Skylark/Dispatch.cpp:1:
/home/klugier/upp/uppsrc/Core/AString.hpp: In member function ‘void Upp::Http::Dispatch(Upp::TcpSocket&)’:
/home/klugier/upp/uppsrc/Core/AString.hpp:171:15: warning: ‘int __builtin_memcmp_eq(const void*, const void*, long unsigned int)’ reading 33 bytes from a region of size 16 [-Wstringop-overflow=]
171 | return memcmp(s, B::Begin(), len * sizeof(tchar)) == 0;
|
Potentially all places with memcpy are affected in AString.hpp not onlu StartsWith, but EndsWith, ReversFind etc..
#18 Updated by Zbigniew Rebacz over 3 years ago
- Status changed from New to Approved
- Assignee changed from Miroslav Fidler to Zbigniew Rebacz
This is not obvious warning and probably bug in gcc. We could close it for now.