Bug 236036

Created attachment 450689 [details] [fast-cq] Patch

Committed r288990 (246709@main): <https://commits.webkit.org/246709@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 450689 [details].

Comment on attachment 450689 [details] [fast-cq] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450689&action=review > Source/WebCore/accessibility/atspi/AccessibilityAtspi.cpp:707 > - static constexpr SortedArrayMap roleNamesMap { roleNames }; > + static const SortedArrayMap roleNamesMap { roleNames }; This change looks like a bad idea. Could you explain why it wasn’t compiling?

(In reply to Darin Adler from comment #3) > Comment on attachment 450689 [details] > [fast-cq] Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=450689&action=review > > > Source/WebCore/accessibility/atspi/AccessibilityAtspi.cpp:707 > > - static constexpr SortedArrayMap roleNamesMap { roleNames }; > > + static const SortedArrayMap roleNamesMap { roleNames }; > > This change looks like a bad idea. Could you explain why it wasn’t compiling? This was the first bug that failed: https://build.webkit.org/#/builders/66/builds/7404 /app/webkit/WebKitBuild/Debug/WTF/Headers/wtf/SortedArrayMap.h: In static member function ‘static const char* WebCore::AccessibilityAtspi::localizedRoleName(WebCore::AccessibilityRole)’: /app/webkit/Source/WebCore/accessibility/atspi/AccessibilityAtspi.cpp:705:62: in ‘constexpr’ expansion of ‘WTF::SortedArrayMap<std::pair<WebCore::AccessibilityRole, WebCore::RoleNameEntry> [122]>(WebCore::roleNames)’ /app/webkit/WebKitBuild/Debug/WTF/Headers/wtf/Assertions.h:364:34: error: call to non-‘constexpr’ function ‘void WTFReportAssertionFailure(const char*, int, const char*, const char*)’ 364 | WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, #assertion); \ | ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /app/webkit/WebKitBuild/Debug/WTF/Headers/wtf/SortedArrayMap.h:151:5: note: in expansion of macro ‘ASSERT_UNDER_CONSTEXPR_CONTEXT’ 151 | ASSERT_UNDER_CONSTEXPR_CONTEXT(isSortedConstExpr(std::begin(array), std::end(array), [] (auto& a, auto b) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /app/webkit/WebKitBuild/Debug/WTF/Headers/wtf/Assertions.h:212:25: note: ‘void WTFReportAssertionFailure(const char*, int, const char*, const char*)’ declared here 212 | WTF_EXPORT_PRIVATE void WTFReportAssertionFailure(const char* file, int line, const char* function, const char* assertion); | ^~~~~~~~~~~~~~~~~~~~~~~~~

And the clang error: In file included from /app/webkit/WebKitBuild/Debug/WebCore/DerivedSources/unified-sources/UnifiedSource-aba958d6-7.cpp:4: /app/webkit/Source/WebCore/accessibility/atspi/AccessibilityAtspi.cpp:707:37: error: constexpr variable 'roleNamesMap' must be initialized by a constant expression static constexpr SortedArrayMap roleNamesMap { roleNames }; ^~~~~~~~~~~~~~~~~~~~~~~~~~ /app/webkit/WebKitBuild/Debug/WTF/Headers/wtf/SortedArrayMap.h:151:5: note: non-constexpr function 'WTFReportAssertionFailure' cannot be used in a constant expression ASSERT_UNDER_CONSTEXPR_CONTEXT(isSortedConstExpr(std::begin(array), std::end(array), [] (auto& a, auto b) { ^ /app/webkit/WebKitBuild/Debug/WTF/Headers/wtf/Assertions.h:364:9: note: expanded from macro 'ASSERT_UNDER_CONSTEXPR_CONTEXT' WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, #assertion); \ ^ /app/webkit/Source/WebCore/accessibility/atspi/AccessibilityAtspi.cpp:707:37: note: in call to 'SortedArrayMap(roleNames)' static constexpr SortedArrayMap roleNamesMap { roleNames }; ^ /app/webkit/WebKitBuild/Debug/WTF/Headers/wtf/Assertions.h:212:25: note: declared here WTF_EXPORT_PRIVATE void WTFReportAssertionFailure(const char* file, int line, const char* function, const char* assertion); ^

So ASSERT_UNDER_CONSTEXPR_CONTEXT is not working on this platform. It’s not working under constexpr context. We need to fix that. Not by changing this line of code, but but fixing the implementation of ASSERT_UNDER_CONSTEXPR_CONTEXT in the Assertions.h header. It’s working on newer versions of clang. We can’t keep dumbing down the rest of our code; even if we turned off those assertions entirely that would be better than un-constexpr'ing code whenever we run into this problem.

The fix might be to change "if" into "if constexpr". But whatever the fix is, the fix has to be a fix to make ASSERT_UNDER_CONSTEXPR_CONTEXT work.

It seems the failure happened because the array being passed to SortedArrayMap wasn't fully ordered, failing the isSortedConstExpr and thus generating the call to WTFReportAssertionFailure and the non-constexpr error. Example of a similar behavior: https://godbolt.org/z/6hzshsvdE In this specific case (build error), fixing the array order makes the build pass. I'll reopen and submit an updated patch reverting the original one. But indeed ASSERT_UNDER_CONSTEXPR_CONTEXT could provide a more meaningful error message in this config.

(In reply to Lauro Moura from comment #8) > It seems the failure happened because the array being passed to > SortedArrayMap wasn't fully ordered, failing the isSortedConstExpr and thus > generating the call to WTFReportAssertionFailure and the non-constexpr error. That makes total sense, great detective work! We have to fix that because it literally might not find some of the items in the map if it’s not sorted. So we definitely need to roll this out. That also means that the assertion will be hit at runtime after patching constexpr to be const. > But indeed ASSERT_UNDER_CONSTEXPR_CONTEXT could provide a more meaningful > error message in this config. I don’t know if that is practical. But we could at least name the WTFReportAssertionFailure function, or a function that is called here, with a clue about what the real problem might be (the assertion failed at compile time!).

(In reply to Darin Adler from comment #9) > So we definitely need to roll this out. Revert this constexpr->const change, I meant.

Created attachment 450698 [details] [fast-cq] Actual fix

Nice found, and really cool that the compile-time assertion can signal that the input is not pre-sorted 🤯️

Committed r289016 (246725@main): <https://commits.webkit.org/246725@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 450698 [details].