Bug 218315 - WebKit should remove unused debug variant support
Summary: WebKit should remove unused debug variant support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-28 16:25 PDT by David Kilzer (:ddkilzer)
Modified: 2020-11-04 13:17 PST (History)
15 users (show)

See Also:


Attachments
Patch v1 (64.36 KB, patch)
2020-10-28 20:18 PDT, David Kilzer (:ddkilzer)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch v2 (86.32 KB, patch)
2020-11-02 10:54 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v3 (77.58 KB, patch)
2020-11-02 14:29 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v4 (77.78 KB, patch)
2020-11-02 14:45 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2020-10-28 16:25:05 PDT
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.
Comment 1 Radar WebKit Bug Importer 2020-10-28 16:25:40 PDT
<rdar://problem/70785369>
Comment 2 David Kilzer (:ddkilzer) 2020-10-28 20:18:14 PDT
Created attachment 412610 [details]
Patch v1
Comment 3 EWS Watchlist 2020-10-28 20:19:25 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 4 David Kilzer (:ddkilzer) 2020-10-28 20:21:19 PDT
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;
Comment 5 Alexey Proskuryakov 2020-10-29 09:55:23 PDT
Is this intended as a replacement for asan.xcconfig? Hopefully, we won't be supporting two ways to enable ASan.
Comment 6 David Kilzer (:ddkilzer) 2020-10-29 12:00:50 PDT
(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.
Comment 7 Darin Adler 2020-10-29 12:05:21 PDT
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.
Comment 8 David Kilzer (:ddkilzer) 2020-11-02 10:19:39 PST
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>
Comment 9 David Kilzer (:ddkilzer) 2020-11-02 10:54:13 PST
Created attachment 412943 [details]
Patch v2
Comment 10 Darin Adler 2020-11-02 11:33:10 PST
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?
Comment 11 David Kilzer (:ddkilzer) 2020-11-02 11:48:32 PST
(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;
Comment 12 Darin Adler 2020-11-02 11:52:52 PST
(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.
Comment 13 David Kilzer (:ddkilzer) 2020-11-02 12:19:49 PST
(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.
Comment 14 Darin Adler 2020-11-02 12:35:05 PST
(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?
Comment 15 David Kilzer (:ddkilzer) 2020-11-02 12:44:32 PST
(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.
Comment 16 Darin Adler 2020-11-02 12:48:15 PST
OK
Comment 17 David Kilzer (:ddkilzer) 2020-11-02 12:50:35 PST
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
Comment 18 Darin Adler 2020-11-02 13:03:27 PST
Eagerly awaiting the new cut at this that removes our use of CURRENT_VARIANT.
Comment 19 David Kilzer (:ddkilzer) 2020-11-02 13:06:32 PST
(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.
Comment 20 David Kilzer (:ddkilzer) 2020-11-02 13:08:59 PST
(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.
Comment 21 Darin Adler 2020-11-02 13:13:09 PST
(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.
Comment 22 David Kilzer (:ddkilzer) 2020-11-02 14:29:00 PST
(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.
Comment 23 David Kilzer (:ddkilzer) 2020-11-02 14:29:53 PST
Created attachment 412970 [details]
Patch v3
Comment 24 David Kilzer (:ddkilzer) 2020-11-02 14:44:25 PST
(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
Comment 25 David Kilzer (:ddkilzer) 2020-11-02 14:45:41 PST
Created attachment 412973 [details]
Patch v4
Comment 26 David Kilzer (:ddkilzer) 2020-11-04 08:15:07 PST
(In reply to David Kilzer (:ddkilzer) from comment #25)
> Created attachment 412973 [details]
> Patch v4

Ping.
Comment 27 EWS 2020-11-04 13:17:57 PST
Committed r269380: <https://trac.webkit.org/changeset/269380>

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