Bug 203492 – [GTK][WPE] Fix various non-unified build issues introduced since r251436

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adrian Perez]

There's some more to churn, in particular after the JSGlobalObject
changes were merged recently in JavaScriptCore :-}

[Adrian Perez]

Created attachment 382082 [details]
Patch

[Alex Christensen]

Comment on attachment 382082 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382082&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        [GTK][WPE] Fix non-unified builds after r251436

This isn't caused by r251436

> Source/JavaScriptCore/runtime/ObjectInitializationScope.h:32
> +#ifdef NDEBUG
> +#include "VM.h"

Let's try to avoid protecting #includes with #ifdef NDEBUG, because that would lead to strange debug-only or release-only build failures.

[Adrian Perez]

(In reply to Alex Christensen from comment #2)
> Comment on attachment 382082 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382082&action=review
> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        [GTK][WPE] Fix non-unified builds after r251436
> 
> This isn't caused by r251436

Ah, probably I should reword this: my intention here was to mean
that the patch addresses all the issues introduced by any commits
after non-unified builds were fixed the last time (which is r251436).

How about rewording “Fix various non-unified build issues introduced
since r251436”?

> > Source/JavaScriptCore/runtime/ObjectInitializationScope.h:32
> > +#ifdef NDEBUG
> > +#include "VM.h"
> 
> Let's try to avoid protecting #includes with #ifdef NDEBUG, because that
> would lead to strange debug-only or release-only build failures.

I will remove the guards before landing.

Thanks for the review!

[Mark Lam]

Comment on attachment 382082 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382082&action=review

>> Source/JavaScriptCore/runtime/ObjectInitializationScope.h:32
>> +#include "VM.h"
> 
> Let's try to avoid protecting #includes with #ifdef NDEBUG, because that would lead to strange debug-only or release-only build failures.

Can you provide some details about why VM.h is needed here?  What is the error?

[Mark Lam]

(In reply to Mark Lam from comment #4)
> Comment on attachment 382082 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382082&action=review
> 
> >> Source/JavaScriptCore/runtime/ObjectInitializationScope.h:32
> >> +#include "VM.h"
> > 
> > Let's try to avoid protecting #includes with #ifdef NDEBUG, because that would lead to strange debug-only or release-only build failures.
> 
> Can you provide some details about why VM.h is needed here?  What is the
> error?

The reason I ask is because VM.h is a heavy-weight file, and I'm always wary when we #include it in yet another header file.  Just wondering if there are any cheaper solutions for this.

[Adrian Perez]

(In reply to Mark Lam from comment #5)
> (In reply to Mark Lam from comment #4)
> > Comment on attachment 382082 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=382082&action=review
> > 
> > >> Source/JavaScriptCore/runtime/ObjectInitializationScope.h:32
> > >> +#include "VM.h"
> > > 
> > > Let's try to avoid protecting #includes with #ifdef NDEBUG, because that would lead to strange debug-only or release-only build failures.
> > 
> > Can you provide some details about why VM.h is needed here?  What is the
> > error?
> 
> The reason I ask is because VM.h is a heavy-weight file, and I'm always wary
> when we #include it in yet another header file.  Just wondering if there are
> any cheaper solutions for this.

The error is caused from the access to “m_vm” in the ~ObjectInitializationScope()
destructor, so I think there is no way around including “VM.h” unless we move
the destructor to the .cpp file — probably a bad idea as it is marked with
ALWAYS_INLINE (when -DNDEBUG is in use → no debugging → release builds).

The compiler error is:

In file included from ../Source/JavaScriptCore/runtime/ObjectInitializationScope.cpp:27:
../Source/JavaScriptCore/runtime/ObjectInitializationScope.h:46:13: error: member access into incomplete type 'JSC::VM'
        m_vm.heap.mutatorFence();
            ^
../Source/JavaScriptCore/runtime/ArrayBuffer.h:42:7: note: forward declaration of 'JSC::VM'
class VM;
      ^
1 error generated.

[Mark Lam]

(In reply to Adrian Perez from comment #6)
> (In reply to Mark Lam from comment #5)
> > (In reply to Mark Lam from comment #4)
> > > Comment on attachment 382082 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=382082&action=review
> > > 
> > > >> Source/JavaScriptCore/runtime/ObjectInitializationScope.h:32
> > > >> +#include "VM.h"
> > > > 
> > > > Let's try to avoid protecting #includes with #ifdef NDEBUG, because that would lead to strange debug-only or release-only build failures.
> > > 
> > > Can you provide some details about why VM.h is needed here?  What is the
> > > error?
> > 
> > The reason I ask is because VM.h is a heavy-weight file, and I'm always wary
> > when we #include it in yet another header file.  Just wondering if there are
> > any cheaper solutions for this.
> 
> The error is caused from the access to “m_vm” in the
> ~ObjectInitializationScope()
> destructor, so I think there is no way around including “VM.h” unless we move
> the destructor to the .cpp file — probably a bad idea as it is marked with
> ALWAYS_INLINE (when -DNDEBUG is in use → no debugging → release builds).
> 
> The compiler error is:
> 
> In file included from
> ../Source/JavaScriptCore/runtime/ObjectInitializationScope.cpp:27:
> ../Source/JavaScriptCore/runtime/ObjectInitializationScope.h:46:13: error:
> member access into incomplete type 'JSC::VM'
>         m_vm.heap.mutatorFence();
>             ^
> ../Source/JavaScriptCore/runtime/ArrayBuffer.h:42:7: note: forward
> declaration of 'JSC::VM'
> class VM;
>       ^
> 1 error generated.

Thanks.  Go ahead and #include it for now.  We can look at an alternative solution later if needed.

[Adrian Perez]

Created attachment 382141 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 382141 [details]:

media/modern-media-controls/compact-media-controls/compact-media-controls-constructor.html bug 193587 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 382141 [details]
Patch for landing

Clearing flags on attachment: 382141

Committed r251690: <https://trac.webkit.org/changeset/251690>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56692913>