Bug 235336 - Fix -Wreturn-type and -Wunused-parameter warnings, January 2022 edition
Summary: Fix -Wreturn-type and -Wunused-parameter warnings, January 2022 edition
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-18 15:25 PST by Michael Catanzaro
Modified: 2022-02-16 08:14 PST (History)
19 users (show)

See Also:

Attachments
Patch (11.27 KB, patch)
2022-01-18 15:26 PST, Michael Catanzaro 		no flags Details | Formatted Diff | Diff
Patch (10.81 KB, patch)
2022-01-18 15:53 PST, Michael Catanzaro 		no flags Details | Formatted Diff | Diff
Patch (11.30 KB, patch)
2022-01-18 16:51 PST, Michael Catanzaro 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2022-01-18 15:25:00 PST 
Fix -Wreturn-type and -Wunused-parameter warnings, January 2022 edition
Comment 1 Michael Catanzaro 2022-01-18 15:26:15 PST 
Created attachment 449433 [details]
Patch
Comment 2 Michael Catanzaro 2022-01-18 15:47:28 PST 
It seems older GCC doesn't like RELEASE_ASSERT_NOT_REACHED() inside constexpr functions.
Comment 3 Darin Adler 2022-01-18 15:53:49 PST 
Comment on attachment 449433 [details]
Patch

Can’t use RELEASE_ASSERT_NOT_REACHED in constexpr functions. RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT does work, although we don’t have a straight "NOT_REACHED" version, so we could do the inelegant RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(false)
Comment 4 Michael Catanzaro 2022-01-18 15:53:54 PST 
Created attachment 449439 [details]
Patch
Comment 5 Michael Catanzaro 2022-01-18 16:19:26 PST 
RELEASE_ASSERT_NOT_REACHED() actually does work fine there with GCC 11.

> RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT

Oooh, didn't know about that. My current patch uses IGNORE_RETURN_TYPE_WARNINGS_BEGIN/END, but that sounds nicer.
Comment 6 Michael Catanzaro 2022-01-18 16:51:36 PST 
Created attachment 449451 [details]
Patch
Comment 7 EWS 2022-01-19 07:57:08 PST 
Committed r288200 (246172@main): <https://commits.webkit.org/246172@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449451 [details].
Comment 8 Radar WebKit Bug Importer 2022-01-19 08:42:11 PST 
<rdar://problem/87772730>
Comment 9 Mikhail R. Gadelha 2022-02-16 05:01:27 PST 
Hi Michael,

EWS MIPS bots use gcc 8.4.0 and this patch breaks the build in debug mode... could you take a look?
Comment 10 Mikhail R. Gadelha 2022-02-16 05:14:00 PST 
The error message:

In file included from WTF/Headers/wtf/StdLibExtras.h:33,
                 from WTF/Headers/wtf/FastMalloc.h:26,
                 from ../../Source/JavaScriptCore/config.h:38,
                 from ../../Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.cpp:26,
                 from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-cd2e8cfa-2.cpp:1:
../../Source/JavaScriptCore/wasm/WasmCompilationMode.h: In function ‘constexpr bool JSC::Wasm::isOSREntry(JSC::Wasm::CompilationMode)’:
WTF/Headers/wtf/Assertions.h:364:34: error: call to non-‘constexpr’ function ‘void WTFReportAssertionFailure(const char*, int, const char*, const char*)’
         WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, #assertion); \
         ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
WTF/Headers/wtf/Assertions.h:661:59: note: in expansion of macro ‘ASSERT_UNDER_CONSTEXPR_CONTEXT’
 #define RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(assertion) ASSERT_UNDER_CONSTEXPR_CONTEXT(assertion)
                                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/JavaScriptCore/wasm/WasmCompilationMode.h:53:5: note: in expansion of macro ‘RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT’
     RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(false);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/JavaScriptCore/wasm/WasmCompilationMode.h: In function ‘constexpr bool JSC::Wasm::isAnyBBQ(JSC::Wasm::CompilationMode)’:
WTF/Headers/wtf/Assertions.h:364:34: error: call to non-‘constexpr’ function ‘void WTFReportAssertionFailure(const char*, int, const char*, const char*)’
         WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, #assertion); \
         ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
WTF/Headers/wtf/Assertions.h:661:59: note: in expansion of macro ‘ASSERT_UNDER_CONSTEXPR_CONTEXT’
 #define RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(assertion) ASSERT_UNDER_CONSTEXPR_CONTEXT(assertion)
                                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/JavaScriptCore/wasm/WasmCompilationMode.h:68:5: note: in expansion of macro ‘RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT’
     RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(false);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/JavaScriptCore/wasm/WasmCompilationMode.h: In function ‘constexpr bool JSC::Wasm::isAnyOMG(JSC::Wasm::CompilationMode)’:
WTF/Headers/wtf/Assertions.h:364:34: error: call to non-‘constexpr’ function ‘void WTFReportAssertionFailure(const char*, int, const char*, const char*)’
         WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, #assertion); \
         ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
WTF/Headers/wtf/Assertions.h:661:59: note: in expansion of macro ‘ASSERT_UNDER_CONSTEXPR_CONTEXT’
 #define RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(assertion) ASSERT_UNDER_CONSTEXPR_CONTEXT(assertion)
                                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/JavaScriptCore/wasm/WasmCompilationMode.h:83:5: note: in expansion of macro ‘RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT’
     RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(false);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Comment 11 Michael Catanzaro 2022-02-16 08:09:59 PST 
Hi Mikhail, that should have been fixed in bug #235401.
Comment 12 Michael Catanzaro 2022-02-16 08:14:51 PST 
Ah, actually I see that fixed RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(), but your problem is with the normal ASSERT_UNDER_CONSTEXPR_CONTEXT(). The problem is identical, though: the assert just doesn't work at all.

I think we simply need to remove the call to WTFReportAssertionFailure() from ASSERT_UNDER_CONSTEXPR_CONTEXT(), since it's not constexpr. Does it build with that change? If so, please report a bug and I'll see if anybody objects to that....