Bug #2077: String0::IsEqual causing warnings with newer GCC - U++ - Redmine

Bug #2077

String0::IsEqual causing warnings with newer GCC

Added by Zbigniew Rebacz over 4 years ago. Updated over 4 years ago.

Status:

Approved

Start date:

09/25/2020

Priority:

Normal

Due date:

Assignee:

Zbigniew Rebacz

% Done:

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 4 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 4 years ago

For record, no warnings with gcc 7.5.0

#3 Updated by Zbigniew Rebacz over 4 years ago

This is gcc 10.2

#4 Updated by Zbigniew Rebacz over 4 years ago

Description updated (diff)

#5 Updated by Zbigniew Rebacz over 4 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 4 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 4 years ago

Without == 0!!!!

memeq returns true if equal...

#8 Updated by Zbigniew Rebacz over 4 years ago

Works fine without comparison and doesn't produce warning.

#9 Updated by Miroslav Fidler over 4 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 4 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 4 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 4 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 4 years ago

Ah, ok, it is in release only...

#14 Updated by Zbigniew Rebacz over 4 years ago

Yes, I compile TheIDE in release mode. GCC already updated :)

#15 Updated by Miroslav Fidler over 4 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 4 years ago

Status changed from Ready for QA to Approved

No warnings anymore - good job :)

#17 Updated by Zbigniew Rebacz over 4 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 4 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.

Also available in: Atom PDF

U++

Issues

Custom queries