Bug 218315 – WebKit should remove unused debug variant support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[David Kilzer (:ddkilzer)]

WebKit should support building with variants other than debug and normal.

We want to be able to build with BUILD_VARIANTS=asan, but Base.xcconfig in most projects doesn't provide a fallback to sensible values for DEBUG_DEFINES, GCC_OPTIMIZATION_LEVEL, STRIP_INSTALLED_PRODUCT and DEAD_CODE_STRIPPING in that situation.

[Radar WebKit Bug Importer]

<rdar://problem/70785369>

[David Kilzer (:ddkilzer)]

Created attachment 412610 [details]
Patch v1

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[David Kilzer (:ddkilzer)]

Comment on attachment 412610 [details]
Patch v1

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

> Source/JavaScriptCore/Configurations/Base.xcconfig:148
> -DEBUG_DEFINES_debug = ;
> -DEBUG_DEFINES_normal = NDEBUG;
> -DEBUG_DEFINES = $(DEBUG_DEFINES_$(CURRENT_VARIANT));
> +DEBUG_DEFINES = $(DEBUG_DEFINES_$(DEBUG_DEFINES_VARIANT_$(CURRENT_VARIANT)));
> +DEBUG_DEFINES_ = NDEBUG;
> +DEBUG_DEFINES_VARIANT_debug = ;

NOTE: I could also define the values this way, but it includes an extra line for each variable, and I wasn't sure it was much clearer:

DEBUG_DEFINES = $(DEBUG_DEFINES_$(DEBUG_DEFINES_VARIANT_$(CURRENT_VARIANT)));
DEBUG_DEFINES_ = $(DEBUG_DEFINES_VARIANT_normal);
DEBUG_DEFINES_VARIANT_debug = ;
DEBUG_DEFINES_VARIANT_normal = NDEBUG;

[Alexey Proskuryakov]

Is this intended as a replacement for asan.xcconfig? Hopefully, we won't be supporting two ways to enable ASan.

[David Kilzer (:ddkilzer)]

(In reply to Alexey Proskuryakov from comment #5)
> Is this intended as a replacement for asan.xcconfig? Hopefully, we won't be
> supporting two ways to enable ASan.

It is a different way to build ASan roots, but it's also a more general fix for supporting multiple build variants in WebKit projects.  (I'm also cleaning up some cruft and inconsistencies between how projects are set up while making this change.)

I don't think we should commingle this fix from deciding whether to use BUILD_VARIANTS=asan with Tools/sanitizers/asan.xcconfig for a few reasons:

1. The ASan roots built using BUILD_VARIANTS=asan will have an _asan suffix, which would require more changes to bot infrastructure to run DRT/WKTR with DYLD_IMAGE_SUFFIX=_asan, so combining them is a multi-step process that would block progress of another feature (or simply opt-out WebKit from the feature).

2. There are toolchain fixes needed to make BUILD_VARIANTS=asan work, so this can't be applied to older SDKs.

3. These changes will otherwise have no impact on any of WebKit's existing builds.

[Darin Adler]

Comment on attachment 412610 [details]
Patch v1

The basics approach of having "debug" variant be the special case and all other variants share a default seems fine.

[David Kilzer (:ddkilzer)]

NOTE: This build fix was needed before posting "Patch v2":

Fix link error with WebKit.framework
<https://trac.webkit.org/r269251>

This fix seemed generic enough that I thought it should be landed separately.

Also note that Source/WebKit/WebProcess/GPU/media/RemoteLegacyCDMSession.cpp has its own copy of convertToUint8Array() (but a slightly different implementation!), and already includes the same header.  Filed this:

Bug 218452: Duplicate copies of static RefPtr<Uint8Array> convertToUint8Array(IPC::SharedBufferCopy&& buffer) in RemoteLegacyCDMSession{Proxy}.cpp
<https://bugs.webkit.org/show_bug.cgi?id=218452>

[David Kilzer (:ddkilzer)]

Created attachment 412943 [details]
Patch v2

[Darin Adler]

Comment on attachment 412943 [details]
Patch v2

I don’t understand why this is both checking CURRENT_VARIANT and [config=Debug].  Why do we need both?

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #10)
> Comment on attachment 412943 [details]
> Patch v2
> 
> I don’t understand why this is both checking CURRENT_VARIANT and
> [config=Debug].  Why do we need both?

One might be tempted to think that CURRENT_VARIANT=debug when building our Debug configuration.  It is not.  Debug, Release and Production builds are all built with CURRENT_VARIANT=normal (and BUILD_VARIANTS=normal).

Also, we would not want CURRENT_VARIANT=debug when building our Debug configuration (without additional changes) because that would add a "_debug" suffix to frameworks and dylib output files so that we would end up with files like /S/L/F/WebKit.framework/WebKit_debug and .../libwebrtc_debug.dylib.

Sorry if this wasn't clear before.  I should also note that while support for the "debug" build variant was added many years ago, we aren't currently building the debug variant internally.

So the [config=Debug] lines exist because I moved the variables out of the Xcode project into the *.xcconfig files, which was one of the last set of variables that we had to include in the Xcode project itself.

Even if we got rid of the CURRENT_VARIANT checks (we would do this in case we don't care about building the _debug variant anymore), we would still need to define the variables like this (example from Source/bmalloc/Configurations/Base.xcconfig):

DEAD_CODE_STRIPPING = YES;
DEAD_CODE_STRIPPING[config=Debug] = NO;

DEBUG_DEFINES = NDEBUG;
DEBUG_DEFINES[config=Debug] = ;

GCC_OPTIMIZATION_LEVEL = 3;
GCC_OPTIMIZATION_LEVEL[config=Debug] = 0;

STRIP_INSTALLED_PRODUCT = YES;
STRIP_INSTALLED_PRODUCT[config=Debug] = NO;

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #11)
> Even if we got rid of the CURRENT_VARIANT checks (we would do this in case
> we don't care about building the _debug variant anymore)

Should we do this? I think that would be a great way to solve the ASan problem. Removing something unused is better than "fixing" it but not committing to testing that it works since no one is using it.

> we would still
> need to define the variables like this (example from
> Source/bmalloc/Configurations/Base.xcconfig):
> 
> DEAD_CODE_STRIPPING = YES;
> DEAD_CODE_STRIPPING[config=Debug] = NO;
> 
> DEBUG_DEFINES = NDEBUG;
> DEBUG_DEFINES[config=Debug] = ;
> 
> GCC_OPTIMIZATION_LEVEL = 3;
> GCC_OPTIMIZATION_LEVEL[config=Debug] = 0;
> 
> STRIP_INSTALLED_PRODUCT = YES;
> STRIP_INSTALLED_PRODUCT[config=Debug] = NO;

OK.

Or we could consider leaving this in the project files, and just fixing it to remove the CURRENT_VARIANT dependency.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #12)
> (In reply to David Kilzer (:ddkilzer) from comment #11)
> > Even if we got rid of the CURRENT_VARIANT checks (we would do this in case
> > we don't care about building the _debug variant anymore)
> 
> Should we do this? I think that would be a great way to solve the ASan
> problem. Removing something unused is better than "fixing" it but not
> committing to testing that it works since no one is using it.

Yes, we can do this.  My original thinking was that there was some value to the _debug variants, but after explaining it, I don't think there's much value since it hasn't been used for years.

> > we would still
> > need to define the variables like this (example from
> > Source/bmalloc/Configurations/Base.xcconfig):
> > 
> > DEAD_CODE_STRIPPING = YES;
> > DEAD_CODE_STRIPPING[config=Debug] = NO;
> > 
> > DEBUG_DEFINES = NDEBUG;
> > DEBUG_DEFINES[config=Debug] = ;
> > 
> > GCC_OPTIMIZATION_LEVEL = 3;
> > GCC_OPTIMIZATION_LEVEL[config=Debug] = 0;
> > 
> > STRIP_INSTALLED_PRODUCT = YES;
> > STRIP_INSTALLED_PRODUCT[config=Debug] = NO;
> 
> OK.
> 
> Or we could consider leaving this in the project files, and just fixing it
> to remove the CURRENT_VARIANT dependency.

The *.xcconfig files were created so we could define build settings outside the project files and to remove duplication.  Moving these settings together (into the same file next to each other) is a progression IMO.

I'll do one more pass to remove the _debug build variant support.

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #13)
> The *.xcconfig files were created so we could define build settings outside
> the project files and to remove duplication.  Moving these settings together
> (into the same file next to each other) is a progression IMO.

What duplication does this remove?

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #14)
> (In reply to David Kilzer (:ddkilzer) from comment #13)
> > The *.xcconfig files were created so we could define build settings outside
> > the project files and to remove duplication.  Moving these settings together
> > (into the same file next to each other) is a progression IMO.
> 
> What duplication does this remove?

Sorry, not being completely clear.

The creation of the *.xcconfig files was to remove duplication and to put the configuration variables in one place.

This most recent changes do not remove duplication, but they do put the variables in one place so you can see what is overriden by just looking at *.xcconfig files instead of both *.xcconfig files and the Xcode project file.  I still think this is an improvement.

[Darin Adler]

OK

[David Kilzer (:ddkilzer)]

Comment on attachment 412943 [details]
Patch v2

FWIW, we use [config=Debug] in other places as well:

$ grep -l -r '\[config=Debug\]' Source Tools
Source/WebKit/SwiftOverlay/Configurations/Base.xcconfig
Source/WebKit/SwiftOverlay/Configurations/WebKitSwiftOverlayTests.xcconfig
Source/ThirdParty/libwebrtc/Configurations/DebugRelease.xcconfig

[Darin Adler]

Eagerly awaiting the new cut at this that removes our use of CURRENT_VARIANT.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #15)
> (In reply to Darin Adler from comment #14)
> > (In reply to David Kilzer (:ddkilzer) from comment #13)
> > > The *.xcconfig files were created so we could define build settings outside
> > > the project files and to remove duplication.  Moving these settings together
> > > (into the same file next to each other) is a progression IMO.
> > 
> > What duplication does this remove?
> 
> Sorry, not being completely clear.
> 
> The creation of the *.xcconfig files was to remove duplication and to put
> the configuration variables in one place.
> 
> This most recent changes do not remove duplication, but they do put the
> variables in one place so you can see what is overriden by just looking at
> *.xcconfig files instead of both *.xcconfig files and the Xcode project
> file.  I still think this is an improvement.

Hmm...need to check what this does in Source/WebKitLegacy/mac/Configurations/Base.xcconfig and others:

GCC_OPTIMIZATION_LEVEL[sdk=iphone*] = 3;
GCC_OPTIMIZATION_LEVEL[sdk=macosx*] = 2;
GCC_OPTIMIZATION_LEVEL[config=Debug] = 0;

I think `config` is more specific than `sdk`, so `config` should win, but I will check.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #18)
> Eagerly awaiting the new cut at this that removes our use of CURRENT_VARIANT.

Working on it now.  I cleared the r? flag on "Patch v2", but I wanted to see if the bots found any other issues with that iteration of the patch that may have been missed in local builds, which is why I didn't obsolete it.

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #19)
> Hmm...need to check what this does in
> Source/WebKitLegacy/mac/Configurations/Base.xcconfig and others:
> 
> GCC_OPTIMIZATION_LEVEL[sdk=iphone*] = 3;
> GCC_OPTIMIZATION_LEVEL[sdk=macosx*] = 2;
> GCC_OPTIMIZATION_LEVEL[config=Debug] = 0;
> 
> I think `config` is more specific than `sdk`, so `config` should win, but I
> will check.

I think that ordering is what makes this work, not specificity. I think later ones override earlier ones.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #21)
> (In reply to David Kilzer (:ddkilzer) from comment #19)
> > Hmm...need to check what this does in
> > Source/WebKitLegacy/mac/Configurations/Base.xcconfig and others:
> > 
> > GCC_OPTIMIZATION_LEVEL[sdk=iphone*] = 3;
> > GCC_OPTIMIZATION_LEVEL[sdk=macosx*] = 2;
> > GCC_OPTIMIZATION_LEVEL[config=Debug] = 0;
> > 
> > I think `config` is more specific than `sdk`, so `config` should win, but I
> > will check.
> 
> I think that ordering is what makes this work, not specificity. I think
> later ones override earlier ones.

I'm not sure about that.  Within an xcconfig file, you can use a variable before it's even defined, and it still works.

Either way, it won't hurt to put [config=Debug] last.

[David Kilzer (:ddkilzer)]

Created attachment 412970 [details]
Patch v3

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #22)
> (In reply to Darin Adler from comment #21)
> > (In reply to David Kilzer (:ddkilzer) from comment #19)
> > > Hmm...need to check what this does in
> > > Source/WebKitLegacy/mac/Configurations/Base.xcconfig and others:
> > > 
> > > GCC_OPTIMIZATION_LEVEL[sdk=iphone*] = 3;
> > > GCC_OPTIMIZATION_LEVEL[sdk=macosx*] = 2;
> > > GCC_OPTIMIZATION_LEVEL[config=Debug] = 0;
> > > 
> > > I think `config` is more specific than `sdk`, so `config` should win, but I
> > > will check.
> > 
> > I think that ordering is what makes this work, not specificity. I think
> > later ones override earlier ones.
> 
> I'm not sure about that.  Within an xcconfig file, you can use a variable
> before it's even defined, and it still works.
> 
> Either way, it won't hurt to put [config=Debug] last.

I was a bit too hasty with "Patch v3" (didn't want to hold up review on my slow builds), but we actually have to do this for it to work in practice:

GCC_OPTIMIZATION_LEVEL[sdk=iphone*] = 3;
GCC_OPTIMIZATION_LEVEL[sdk=iphone*][config=Debug] = 0;
GCC_OPTIMIZATION_LEVEL[sdk=macosx*] = 2;
GCC_OPTIMIZATION_LEVEL[sdk=macosx*][config=Debug] = 0;

Only affects three files:

Source/WebCore/Configurations/Base.xcconfig
Source/WebCore/PAL/Configurations/Base.xcconfig
Source/WebKitLegacy/mac/Configurations/Base.xcconfig

[David Kilzer (:ddkilzer)]

Created attachment 412973 [details]
Patch v4

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #25)
> Created attachment 412973 [details]
> Patch v4

Ping.

[EWS]

Committed r269380: <https://trac.webkit.org/changeset/269380>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412973 [details].