| Summary: | Debug build failure after r246172: ASSERT_UNDER_CONSTEXPR_CONTEXT should work in constexpr contexts | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mikhail R. Gadelha <mikhail> | ||||||||||||||||||
| Component: | New Bugs | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | benjamin, bugs-noreply, cdumez, cmarcelo, darin, ews-watchlist, guijemont, keith_miller, lmoura, mark.lam, mcatanzaro, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
Michael, while removing WTFReportAssertionFailure does fix the compilation, we'll also lose the location information from the assertion. Maybe we can find another solution? Something using RELEASE_ASSERT_NOT_REACHED() perhaps? (In reply to Mikhail R. Gadelha from comment #1) > Michael, while removing WTFReportAssertionFailure does fix the compilation, Happy to r+ such a patch if you want to go that route. > we'll also lose the location information from the assertion. Indeed, but that's hardy a serious loss IMO. If you want to figure out how to implement a constexpr print function that would placate GCC 8, go ahead. I don't plan to try. (Honestly, I'm not sure if it's even possible.) > Maybe we can find another solution? Something using > RELEASE_ASSERT_NOT_REACHED() perhaps? That's not constexpr either. I encountered the issue when building for armv7 as well. So it's not mips specific, probably 32-bit specific? It probably happens to anyone using GCC 8. Let's go with the easy solution. Created attachment 452517 [details]
Patch
Comment on attachment 452517 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452517&action=review > Source/WTF/wtf/Assertions.h:-364 > - WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, #assertion); \ I worry about a cross-platform change to fix a gcc-specific error. I agree, and it's weird that gcc still complains even though all the enum values are covered.
How about we change the switch to:
constexpr inline bool isOSREntry(CompilationMode compilationMode)
{
if (compilationMode == CompilationMode::OMGForOSREntryMode ||
compilationMode == CompilationMode::BBQForOSREntryMode)
return true;
RELEASE_ASSERT(compilationMode == CompilationMode::LLIntMode ||
compilationMode == CompilationMode::BBQMode ||
compilationMode == CompilationMode::OMGMode ||
compilationMode == CompilationMode::EmbedderEntrypointMode);
return false;
}
It's not pretty but gcc 8.4.0 doesn't seem to complain.
(In reply to Darin Adler from comment #6) > I worry about a cross-platform change to fix a gcc-specific error. That's why I CCed you, but as far as I can tell, the implementation of ASSERT_UNDER_CONSTEXPR_CONTEXT() is just wrong... WTFReportAssertionFailure is not constexpr (and it cannot be, either) and so it cannot be called in a constexpr context, right? This seems very similar to how we recently discovered that RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT() was not constexpr either. (In reply to Michael Catanzaro from comment #8) > (In reply to Darin Adler from comment #6) > > I worry about a cross-platform change to fix a gcc-specific error. > > That's why I CCed you, but as far as I can tell, the implementation of > ASSERT_UNDER_CONSTEXPR_CONTEXT() is just wrong... WTFReportAssertionFailure > is not constexpr (and it cannot be, either) and so it cannot be called in a > constexpr context, right? This seems very similar to how we recently > discovered that RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT() was not constexpr > either. This is a complicated subject. If the assertion fails in a constexpr context, then we want the compile to fail, so it doesn’t matter what kind of function WTFReportAssertionFailure is. If the assertion succeeds in a constexpr context, then the compile must succeed. And if the assertion is used in a function that can work both in a constexpr context and not in a constexpr context, then the call to WTFReportAssertionFailure is valuable because in the non-constexpr context it will work and provide a report. So it’s not as simple as saying "this macro calls a non-constexpr function so it must be wrong". However, there may well be something wrong with my understanding above; I didn’t write the macro and I’m not sure what the person who wrote it was thinking. We do want to be able to write assertions that fail at compile time in a constexpr context, but when they fail at runtime, that report the assertion failure. Whether the macro achieves this or not is unclear to me. It’s possible this is broken. And it’s possible it’s not. Need some testing, I think, to be sure. And it’s possible the changes we are making here and have made recently are ruining the logging when ASSERT_UNDER_CONSTEXPR_CONTEXT is used in a function that works both in and out of constexpr context. Created attachment 452532 [details]
Patch
The patch I just uploaded fixes the missing return warning and avoids using RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT. WDYT Michael and Darin? Comment on attachment 452532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452532&action=review > Source/JavaScriptCore/wasm/WasmCompilationMode.h:47 > + RELEASE_ASSERT(compilationMode == CompilationMode::LLIntMode Um, but you can't use RELEASE_ASSERT in a constexpr function... you have to use RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT. In theory. I'm increasingly confused why compilers seem to sometimes care and sometimes not.... Created attachment 452537 [details]
Patch
New one with RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT, let's see what EWS says. Comment on attachment 452537 [details]
Patch
This change is OK, but looking at it closer: honestly, just remove the RELEASE_ASSERT altogether, it will be simpler that way. I added those in order to silence the -Wreturn-type warning that happened without it when the code used switch statements. With the switches removed, there's no longer any need for the assert anymore. That will simplify these functions considerably.
That said, I think we really need to ensure that RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT() actually works reliably.
We should keep the assertion to prevent people from adding new enum values and forgetting to update these functions. Of course the best way to ensure nobody forgets to add new enum values is to simply not remove the switch statements.... (In reply to Michael Catanzaro from comment #17) > Of course the best way to ensure nobody forgets to add new enum values is to > simply not remove the switch statements.... There is no arguing that, but it was the only way I've found to prevent the return-type warning while keeping WTFReportAssertionFailure on GCC 8.4.0 Comment on attachment 452537 [details]
Patch
Ah, I finally understand the problem here. RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT() builds fine as long as the assert does not trigger at build time. But if it does trigger, then the build fails. Older GCC fails when it sees RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(false). So the assert *usually* works, and only calling the macro with false is broken.
I would rewrite switch -> if as you've done here, and just remove the asserts altogether. They're really not accomplishing much here. Alternatively, you could add a RELEASE_ASSERT_NOT_REACHED_UNDER_CONSTEXPR_CONTEXT() that does not print anything and would bypass the build failure problem. Whichever you prefer.
(In reply to Michael Catanzaro from comment #19) > Comment on attachment 452537 [details] > Patch > > Ah, I finally understand the problem here. > RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT() builds fine as long as the assert > does not trigger at build time. But if it does trigger, then the build > fails. Older GCC fails when it sees > RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(false). So the assert *usually* > works, and only calling the macro with false is broken. > This seems similar to what happened in bug236036, where an unordered array caused an static assertion in SortedArrayMap constructor and the compiler complained about nonconstexpr WTFReport... being called in a constexpr context. Yes: When the assertion fails at compile time, a compile failure is expected. Not like this, though. It's one thing to use static_assert and fail the build with a reasonable error message. This is not even close. Yes, exactly. Can be fixed, we can improve the error message really easily. Just name the function called something easy to understand like IfYouSeedThisAnAssertionFailed, and have it call the other function. (In reply to Michael Catanzaro from comment #19) > Comment on attachment 452537 [details] > Patch > > Ah, I finally understand the problem here. > RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT() builds fine as long as the assert > does not trigger at build time. But if it does trigger, then the build > fails. Older GCC fails when it sees > RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(false). So the assert *usually* > works, and only calling the macro with false is broken. > > I would rewrite switch -> if as you've done here, and just remove the > asserts altogether. They're really not accomplishing much here. > Alternatively, you could add a > RELEASE_ASSERT_NOT_REACHED_UNDER_CONSTEXPR_CONTEXT() that does not print > anything and would bypass the build failure problem. Whichever you prefer. How about changing it to an if and to convert RELEASE_ASSERT_NOT_REACHED_UNDER_CONSTEXPR_CONTEXT() into a ASSERT_NOT_REACHED_UNDER_CONSTEXPR_CONTEXT()? That way we can keep the assertion around for debug builds, so people don't forget to update the code if a new value is added to the enum? Created attachment 454127 [details]
Patch
Comment on attachment 454127 [details]
Patch
My $0.02: the asserts are inappropriate, these functions will work perfectly fine if compilationMode is not one of the values expected by the assert. It's understood that code needs to be updated when adding new enum values....
Created attachment 454238 [details]
Patch
Comment on attachment 454238 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454238&action=review > Source/JavaScriptCore/ChangeLog:9 > + ASSERT_UNDER_CONSTEXPR_CONTEXT is calling WTFReportAssertionFailure, a non-constexpr > + function that cannot be made constexpr because it has to print a message at runtime. Please change the commit message before landing: the problem is ASSERT_UNDER_CONSTEXPR_CONTEXT calls a non-constexpr function if its assertion fails, so it cannot be used for an assert not reached. It's fine for other purposes. Created attachment 454252 [details]
Patch
Comment on attachment 454252 [details]
Patch
Please continue using switch (without default). This is useful to find which code needs to be modified when a new compilation-mode is added. So, if it is related to GCC's warning, please suppress warning.
Can we fix it by suppressing GCC warning here? I think switching `switch` to `if` here is a regression. We add a new type of compilation-mode, and this kind of switch can let us know we missed handling the new one in this code. I think you could replace the assert with use of IGNORE_RETURN_TYPE_WARNINGS_BEGIN and IGNORE_RETURN_TYPE_WARNINGS_END around the entire function. Better: write a RELEASE_ASSERT_NOT_REACHED_UNDER_CONSTEXPR_CONTEXT() macro and omit the print line so that it builds successfully with the affected version of GCC. Yeah, I've talked with Yusuke, and calling CRASH seems like the better solution. The patch will follow shortly. Created attachment 454279 [details]
Patch
Comment on attachment 454279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454279&action=review > Source/WTF/wtf/Assertions.h:384 > +#define ASSERT_NOT_REACHED_UNDER_CONSTEXPR_CONTEXT(...) do { \ > + CRASH(); \ > +} while (0) OK. You need CRASH_UNDER_CONSTEXPR_CONTEXT() here, though. I bet it will work then. Comment on attachment 454279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454279&action=review > Source/WTF/wtf/Assertions.h:675 > +#define RELEASE_ASSERT_NOT_REACHED_UNDER_CONSTEXPR_CONTEXT() ASSERT_NOT_REACHED_UNDER_CONSTEXPR_CONTEXT() You need to define this in the !ASSERT_ENABLED case too, a few lines up. Just directly call CRASH_UNDER_CONSTEXPR_CONTEXT(). Created attachment 454299 [details]
Patch
Comment on attachment 454299 [details]
Patch
r=me
Committed r291165 (248325@main): <https://commits.webkit.org/248325@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 454299 [details]. |
MIPS EWS uses gcc 8.4.0 by default and when trying to build jsc (debug mode) after #235336 we get: 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); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~