Bug 245321 – Ran clang-tidy on JSC, WTF and bmalloc

• Status: REOPENED
• Component: JavaScriptCore

[Mikhail R. Gadelha]

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.

[EWS]

Committed 254605@main (700ac83f17d8): <https://commits.webkit.org/254605@main>

Reviewed commits have been landed. Closing PR #3500 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/100089222>

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

I'll reopen this.

[Yusuke Suzuki]

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

[Mikhail R. Gadelha]

Hey Yusuke, I'll help Nikolas debugging the issue, please wait a couple of days before reverting it.

[Yusuke Suzuki]

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

[Mikhail R. Gadelha]

(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?

[Yusuke Suzuki]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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?

[Yusuke Suzuki]

BTW the change is reverted in https://github.com/WebKit/WebKit/commit/aabfacb8cf9ca4780d20ce2cca233f7988e994e2