Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2556383002:
Don't define log2 and log2f in blink for Android (Closed)
|
Created:
4 years ago by brucedawson Modified:
4 years ago Reviewers:
Nico CC:
chromium-reviews, blink-reviews, blink-reviews-wtf_chromium.org, Mikhail Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't define log2 and log2f in blink for Android
The definition of log2f in blink was causing problems on Windows and
is no longer necessary. It is also unnecessary on Android and may
also be causing problems there, so this change finishes removing
the definitions.
See crrev.com/2556383002 for details.
Committed: https://crrev.com/def20b0a429121024fa0b148ad112787f7edb479
Cr-Commit-Position: refs/heads/master@{#437351}
Patch Set 1 #Patch Set 2 : Merging #Messages
Total messages: 19 (14 generated)
The CQ bit was checked by brucedawson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by brucedawson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Don't define log2 and log2f in blink at all
The consequences are surprising.
A long time ago VC++'s CRT didn't supply log2 and log2f so WebKit/blink
supplied them as inline functions in MathExtras.h. In some cases
(including the full official builds that we ship to customers) this
meant that when Skia called log2f they ended up pulling in blink's
version. This pulled in PeriodicWave.obj which then pulled in various
other object files, and somehow the unreferenced-code logic in the
linker was unable to get rid of all of the rubbish. I noticed this
because during the gyp->gn transition I noticed that ff_cos_131072
and friends were in gn builds of chrome.dll but not gyp builds. I
fixed this but then they came back in full LTCG official builds
(full_wpo_on_official = true). Analysis of a verbose link gave this
chain of events for why rdft.obj (the definer of ff_*) was pulled in:
rdft.obj referenced by
avfft.obj for symbol ff_rdft_init
avfft.obj referenced by
FFTFrameFFMPEG.obj for symbol av_rdft_calc
FFTFrameFFMPEG.obj referenced by
PeriodicWave.obj for symbol public: void __thiscall
blink::FFTFrame::doInverseFFT(float *)
PeriodicWave.obj referenced by
SkGeometry.obj for symbol log2f
Usually the trick to resolving these unwanted symbols is changing a
source_set to static_library, but this time it was recognizing that
SkGeometry has no business pulling in PeriodicWave, and removing the
VC++ definition of log2f that is the culprit. The effect on the size of
chrome.dll is:
chrome.dll
.text: -6656 bytes change
.rdata: 240 bytes change
.data: -786624 bytes change
.rodata: -592 bytes change
.reloc: -872 bytes change
Total change: -794504 bytes
BUG=630755
patch from issue 2559063002 at patchset 1 (http://crrev.com/2559063002#ps1)
==========
to
==========
Don't define log2 and log2f in blink for Android
The definition of log2f in blink was causing problems on Windows and
is no longer necessary. It is also unnecessary on Android and may
also be causing problems there, so this change finishes removing
the definitions.
See crrev.com/2556383002 for details.
==========
brucedawson@chromium.org changed reviewers: + thakis@chromium.org
This is a follow-up change to completely remove these functions. Presumably having this change build successfully on Android is sufficient to prove it is good. I don't know whether it helps, but it should not hurt and is the right thing to do to minimize risk.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481231948322460,
"parent_rev": "0da123422dbc2a7e163a1906e92b2556b9389136", "commit_rev":
"51bc52ccfa126ed3387781435e53f44ab89409d4"}
Message was sent while issue was closed.
Description was changed from ========== Don't define log2 and log2f in blink for Android The definition of log2f in blink was causing problems on Windows and is no longer necessary. It is also unnecessary on Android and may also be causing problems there, so this change finishes removing the definitions. See crrev.com/2556383002 for details. ========== to ========== Don't define log2 and log2f in blink for Android The definition of log2f in blink was causing problems on Windows and is no longer necessary. It is also unnecessary on Android and may also be causing problems there, so this change finishes removing the definitions. See crrev.com/2556383002 for details. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Don't define log2 and log2f in blink for Android The definition of log2f in blink was causing problems on Windows and is no longer necessary. It is also unnecessary on Android and may also be causing problems there, so this change finishes removing the definitions. See crrev.com/2556383002 for details. ========== to ========== Don't define log2 and log2f in blink for Android The definition of log2f in blink was causing problems on Windows and is no longer necessary. It is also unnecessary on Android and may also be causing problems there, so this change finishes removing the definitions. See crrev.com/2556383002 for details. Committed: https://crrev.com/def20b0a429121024fa0b148ad112787f7edb479 Cr-Commit-Position: refs/heads/master@{#437351} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/def20b0a429121024fa0b148ad112787f7edb479 Cr-Commit-Position: refs/heads/master@{#437351} |
