Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround restrict error in GCC 12 #26961

Merged

Conversation

TingPing
Copy link
Contributor

@TingPing TingPing commented Apr 8, 2024

bd1249c

Workaround restrict error in GCC 12
https://bugs.webkit.org/show_bug.cgi?id=272309

Reviewed by Darin Adler.

In GCC 12.3.0:

In static member function ‘static constexpr std::char_traits<char>::char_type* std::char_traits<char>::copy(char_type*, const char_type*, std::size_t)’,
    inlined from ‘static constexpr void std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_S_copy(_CharT*, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/basic_string.h:431:21,
    inlined from ‘constexpr std::__cxx11::basic_string<_CharT, _Traits, _Allocator>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_M_replace(size_type, size_type, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/basic_string.tcc:532:22,
    inlined from ‘constexpr std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::replace(size_type, size_type, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/basic_string.h:2179:19,
    inlined from ‘constexpr std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::insert(size_type, const _CharT*) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/basic_string.h:1936:22,
    inlined from ‘std::ostream& WTF::operator<<(std::ostream&, Int128Impl)’ at /host/home/tingping/Projects/WebKit/Source/WTF/wtf/Int128.cpp:268:17:
/usr/include/c++/12/bits/char_traits.h:435:56: error: ‘void* __builtin_memcpy(void*, const void*, long unsigned int)’ accessing 9223372036854775810 or more bytes at offsets [2, 9223372036854775807] and 1 may overlap up to 9223372036854775813 bytes at offset -3 [-Werror=restrict]
  435 |         return static_cast<char_type*>(__builtin_memcpy(__s1, __s2, __n));
      |                                        ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~

* Source/WTF/wtf/Int128.cpp:
(WTF::operator<<):

Canonical link: https://commits.webkit.org/277203@main

9fd1a25

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-skia
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv-sim ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🛠 watch ✅ 🛠 jsc-armv7
✅ 🛠 watch-sim ✅ 🧪 jsc-armv7-tests

@TingPing TingPing self-assigned this Apr 8, 2024
@TingPing TingPing added the New Bugs Unclassified bugs are placed in this component until the correct component can be determined. label Apr 8, 2024
@TingPing
Copy link
Contributor Author

TingPing commented Apr 8, 2024

Style check failure is because that function already used wrong indentation, this leaves the style as it was.

Comment on lines -268 to +270
rep = "-";
rep.append("-");
} else if (flags & std::ios::showpos) {
rep = "+";
rep.append("+");
Copy link
Member

@darinadler darinadler Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this GCC 12 warning correct, or a bug? If it’s a bug, do you have a link to the GCC bug report?

This doesn’t seem like a great fix, especially unconditionally on all platforms without any comment. But on the other hand, pretty sure this is dead code in WebKit. We don’t use Int128 with an ostream. So we shouldn’t waste any time on this.

Copy link
Contributor Author

@TingPing TingPing Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the bug reported here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105329

Reported 2022 but still unfixed. I'll add the link to the commit message.

I don't believe there is anything wrong with our code at all. But I don't know if GCC is doing the right thing so it could be unsafe there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh it was already merged, well it's documented here :)

@darinadler darinadler added merge-queue Applied to send a pull request to merge-queue and removed New Bugs Unclassified bugs are placed in this component until the correct component can be determined. labels Apr 8, 2024
https://bugs.webkit.org/show_bug.cgi?id=272309

Reviewed by Darin Adler.

In GCC 12.3.0:

In static member function ‘static constexpr std::char_traits<char>::char_type* std::char_traits<char>::copy(char_type*, const char_type*, std::size_t)’,
    inlined from ‘static constexpr void std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_S_copy(_CharT*, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/basic_string.h:431:21,
    inlined from ‘constexpr std::__cxx11::basic_string<_CharT, _Traits, _Allocator>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_M_replace(size_type, size_type, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/basic_string.tcc:532:22,
    inlined from ‘constexpr std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::replace(size_type, size_type, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/basic_string.h:2179:19,
    inlined from ‘constexpr std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::insert(size_type, const _CharT*) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/basic_string.h:1936:22,
    inlined from ‘std::ostream& WTF::operator<<(std::ostream&, Int128Impl)’ at /host/home/tingping/Projects/WebKit/Source/WTF/wtf/Int128.cpp:268:17:
/usr/include/c++/12/bits/char_traits.h:435:56: error: ‘void* __builtin_memcpy(void*, const void*, long unsigned int)’ accessing 9223372036854775810 or more bytes at offsets [2, 9223372036854775807] and 1 may overlap up to 9223372036854775813 bytes at offset -3 [-Werror=restrict]
  435 |         return static_cast<char_type*>(__builtin_memcpy(__s1, __s2, __n));
      |                                        ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~

* Source/WTF/wtf/Int128.cpp:
(WTF::operator<<):

Canonical link: https://commits.webkit.org/277203@main
@webkit-commit-queue
Copy link
Collaborator

Committed 277203@main (bd1249c): https://commits.webkit.org/277203@main

Reviewed commits have been landed. Closing PR #26961 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit bd1249c into WebKit:main Apr 8, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants