Bug 236036 – [GLIB] Unreviewed debug buildfix after r288872

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Lauro Moura]

[GLIB] Unreviewed debug buildfix after r288872

[Lauro Moura]

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

[EWS]

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].

[Darin Adler]

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?

[Lauro Moura]

(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);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~

[Lauro Moura]

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);
                        ^

[Darin Adler]

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.

[Darin Adler]

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.

[Lauro Moura]

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.

[Darin Adler]

(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!).

[Darin Adler]

(In reply to Darin Adler from comment #9)
> So we definitely need to roll this out.

Revert this constexpr->const change, I meant.

[Lauro Moura]

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

[Adrian Perez]

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

[EWS]

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].