Bug #2077

String0::IsEqual causing warnings with newer GCC

Added by Zbigniew Rebacz 2 months ago. Updated about 2 months ago.

Status:ApprovedStart date:09/25/2020
Priority:NormalDue date:
Assignee:Zbigniew Rebacz% Done:

0%

Category:CoreSpent 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 2 months 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 2 months ago

For record, no warnings with gcc 7.5.0

#3 Updated by Zbigniew Rebacz 2 months ago

This is gcc 10.2

#4 Updated by Zbigniew Rebacz 2 months ago

  • Description updated (diff)

#5 Updated by Zbigniew Rebacz 2 months 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 2 months 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 2 months ago

Without == 0!!!!

memeq returns true if equal...

#8 Updated by Zbigniew Rebacz 2 months ago

Works fine without comparison and doesn't produce warning.

#9 Updated by Miroslav Fidler 2 months 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 2 months ago

I use Manjaro KDE edition. This is rolling distro, so you all the time should have decent compilers.

#11 Updated by Miroslav Fidler 2 months 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 2 months ago

  • Status changed from Ready for QA to In Progress
  • Assignee changed from Miroslav Fidler to Zbigniew Rebacz

#13 Updated by Miroslav Fidler 2 months ago

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

#14 Updated by Zbigniew Rebacz 2 months ago

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

#15 Updated by Miroslav Fidler 2 months 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 2 months ago

  • Status changed from Ready for QA to Approved

No warnings anymore - good job :)

#17 Updated by Zbigniew Rebacz about 2 months 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 about 2 months 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