245321 2022-09-17 11:45:13 -0700 Ran clang-tidy on JSC, WTF and bmalloc 2022-10-06 09:44:01 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified REOPENED https://bugs.webkit.org/show_bug.cgi?id=245874 InRadar P2 Normal --- 245874 1 mikhail webkit-unassigned webkit-bug-importer ysuzuki zimmermann oldest_to_newest 1899232 0 mikhail 2022-09-17 11:45:13 -0700 The following checks were used: * modernize-use-default-member-init * readability-redundant-member-init * modernize-use-equals-default * performance-trivially-destructible Some interesting points: 1. No all constants were moved to move inits: stuff like UINT_MAX are not actually constant in the platform that I run clang-tidy (linux 64bit), so these had to be moved manually. 2. modernize-use-equals-default tried to set default to constructors and destructors of unions, which broke compilation. 3. Some constructros were left with an empty body so the compiler doesn't complain about unused variable for classes like AllowUnfinalizedAccessScope 4. In the struct BranchTarget (DFGNode.h), a float (count) was being initialized using a pure NaN (double). While gcc didn't complain when it was done in the constructor initializer list, it started to warn about fp narrowing. So I manually changed the init value to use the NAN macro. 5. Some enums were not moved from the initializer list, I'm not sure why. I did my best to manually move as many as I could find but is highly likely that a few remain. 6. Windows doesn't seem to support default member initializer for a member of an anonymous struct within a union, so these had to be removed. 1899329 1 ews-feeder 2022-09-18 06:43:30 -0700 Committed 254605@main (700ac83f17d8): <https://commits.webkit.org/254605@main> Reviewed commits have been landed. Closing PR #3500 and removing active labels. 1899330 2 webkit-bug-importer 2022-09-18 06:44:16 -0700 <rdar://problem/100089222> 1902415 3 zimmermann 2022-09-30 12:11:49 -0700 This caused a regression on ARM 32bit -- SIGILL during WebProcess startup. I've split this patch into multiple pieces, WebKit main + only the bmalloc/ portion of this patch is enough to have WebProcess constantly abort with SIGILL. It's hard to produce actual stacktraces, as I have to selectively build certain files with debug symbols, a full WebKit debug or RelWithDebInfo builds leads to large libWPEWebKit.so, exceeding > 2GB, not possible to use as-is on embedded. Anyhow, I'm 100% certain by bisect that this commit is guilty and we're likely facing a compiler problem, since by code inspection I cannot find anything suspicious in the clang-tidy-patch itself. However the patch already triggered a debug compilation problem with clang10, where KeyValuePairs "= default" constructor leads to a compilation error, whereas using " { }" instead works fine :/ Looking forward to nail down the culprit within the Source/bmalloc/ changes. 1902416 4 zimmermann 2022-09-30 12:12:13 -0700 I'll reopen this. 1903597 5 ysuzuki 2022-10-05 16:41:16 -0700 (In reply to Nikolas Zimmermann from comment #3) > This caused a regression on ARM 32bit -- SIGILL during WebProcess startup. > I've split this patch into multiple pieces, WebKit main + only the bmalloc/ > portion of this patch is enough to have WebProcess constantly abort with > SIGILL. > > It's hard to produce actual stacktraces, as I have to selectively build > certain files with debug symbols, a full WebKit debug or RelWithDebInfo > builds leads to large libWPEWebKit.so, exceeding > 2GB, not possible to use > as-is on embedded. > > Anyhow, I'm 100% certain by bisect that this commit is guilty and we're > likely facing a compiler problem, since by code inspection I cannot find > anything suspicious in the clang-tidy-patch itself. > > However the patch already triggered a debug compilation problem with > clang10, where KeyValuePairs "= default" constructor leads to a compilation > error, whereas using " { }" instead works fine :/ > > Looking forward to nail down the culprit within the Source/bmalloc/ changes. Let's revert this. 1903613 6 mikhail 2022-10-05 17:13:06 -0700 Hey Yusuke, I'll help Nikolas debugging the issue, please wait a couple of days before reverting it. 1903614 7 ysuzuki 2022-10-05 17:13:38 -0700 (In reply to Mikhail R. Gadelha from comment #6) > Hey Yusuke, I'll help Nikolas debugging the issue, please wait a couple of > days before reverting it. It is not OK. The latest watchOS is build is already broken, which needs to be fixed ASAP. 1903619 8 mikhail 2022-10-05 17:19:57 -0700 (In reply to Yusuke Suzuki from comment #7) > (In reply to Mikhail R. Gadelha from comment #6) > > Hey Yusuke, I'll help Nikolas debugging the issue, please wait a couple of > > days before reverting it. > > It is not OK. The latest watchOS is build is already broken, which needs to > be fixed ASAP. I'll work on it. Also, is there a log of the watchOS crash? 1903622 9 ysuzuki 2022-10-05 17:23:48 -0700 (In reply to Mikhail R. Gadelha from comment #8) > (In reply to Yusuke Suzuki from comment #7) > > (In reply to Mikhail R. Gadelha from comment #6) > > > Hey Yusuke, I'll help Nikolas debugging the issue, please wait a couple of > > > days before reverting it. > > > > It is not OK. The latest watchOS is build is already broken, which needs to > > be fixed ASAP. > > I'll work on it. > > Also, is there a log of the watchOS crash? I'm seeing the changes in detail right now, and seeing a bit wrong changes in various places. For example, JSC::UnlinkedFunctionExecutable::m_features' initialization look missing. Unused `m_spillStateForJSGetterSetter` field is added in AccessGenerationState, etc. So you need to split the changes and we need careful review for each change. I'll revert it now. 1903689 10 zimmermann 2022-10-06 01:33:24 -0700 (In reply to Yusuke Suzuki from comment #9) > (In reply to Mikhail R. Gadelha from comment #8) > > (In reply to Yusuke Suzuki from comment #7) > > > (In reply to Mikhail R. Gadelha from comment #6) > > > > Hey Yusuke, I'll help Nikolas debugging the issue, please wait a couple of > > > > days before reverting it. > > > > > > It is not OK. The latest watchOS is build is already broken, which needs to > > > be fixed ASAP. > > > > I'll work on it. > > > > Also, is there a log of the watchOS crash? > > I'm seeing the changes in detail right now, and seeing a bit wrong changes > in various places. For example, JSC::UnlinkedFunctionExecutable::m_features' > initialization look missing. Unused `m_spillStateForJSGetterSetter` field is > added in AccessGenerationState, etc. > So you need to split the changes and we need careful review for each change. > I'll revert it now. Thanks Yusuke, I've aleady had a hard time splitting up the patch into more smaler pieces, to continue my bisect within the patch... :/ I agree it is easier to revert it, and reland smaller -- after the SIGILL is understood. 1903705 11 zimmermann 2022-10-06 02:49:31 -0700 I've nailed this down to either the change to Gigacache.cpp or IsoDirectory.h. Background: I reverted the whole patch, and only added Source/bmalloc changes - SIGILL fired. Then reverted file-by-file until only 3 files were left. Then reverted the changes to Gigacache.cpp / IsoDirectory.h and the SIGILL is gone. clang10 choked also on debug builds on another file where s/= default;/= {}/ fixed the build -- from that I suspect clang10 has an issue with these contstructs, therefore most likely IsoDirectory.h is causing some misbehavior / compiler bug? 1903779 12 ysuzuki 2022-10-06 09:44:01 -0700 BTW the change is reverted in https://github.com/WebKit/WebKit/commit/aabfacb8cf9ca4780d20ce2cca233f7988e994e2