Migrate some case conversion functions from JS to CPP builtins

Migrate String.prototype.to{Upper,Lower}Case functions from JS to CPP builtins.

Move ICU case conversion utility functions to a common location.

BUG=v8:5751
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng

Review-Url: https://codereview.chromium.org/2728763006
Cr-Commit-Position: refs/heads/master@{#44050}
Committed: https://chromium.googlesource.com/v8/v8/+/4a5d1e253573d98fb368f5575412d62e07ee1cf3

[Dan Ehrenberg, 2017-03-14 13:51:15]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 13:51:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 14:11:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 14:11:39]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/18973)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[caitp, 2017-03-14 14:41:33]

On 2017/03/14 14:11:39, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/18973)
>   v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

There are a few problems I see from a quick look:

InstallBuiltinFunctionIds() crashes because toLowerCase and toUpperCase are no
longer included in the snapshot. The unibrow version should be present in the
snapshot, at the very least. You can use SetFunction rather than InstallFunction
to avoid the DCHECK failure if the function already exists.

That's basically it, I'm pretty sure this will work if you move unibrow case
conversion functions back into the snapshot.

[caitp, 2017-03-14 15:08:24]

On 2017/03/14 14:41:33, caitp wrote:
> On 2017/03/14 14:11:39, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
> >
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/18973)
> >   v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
> 
> There are a few problems I see from a quick look:
> 
> InstallBuiltinFunctionIds() crashes because toLowerCase and toUpperCase are no
> longer included in the snapshot. The unibrow version should be present in the
> snapshot, at the very least. You can use SetFunction rather than
InstallFunction
> to avoid the DCHECK failure if the function already exists.
> 
> That's basically it, I'm pretty sure this will work if you move unibrow case
> conversion functions back into the snapshot.

https://gist.github.com/caitp/5ec98cc73e091ff50a333c87a685e4c5 avoids the
crashes, but still fails  tests, I haven't delved into why (it's the same tests
failing on the bots atm)

[Dan Ehrenberg, 2017-03-14 16:19:56]

https://codereview.chromium.org/2728763006/diff/20001/src/builtins/builtins-i...
File src/builtins/builtins-intl.cc (right):

https://codereview.chromium.org/2728763006/diff/20001/src/builtins/builtins-i...
src/builtins/builtins-intl.cc:126: MUST_USE_RESULT static Object* ConvertCase(
It looks like this case conversion code was copied from
src/builtins/builtins-string.cc . Instead, we should be using the case
conversion code in src/runtime/runtime-i18n.cc . This ConvertCase function has
the same name, but it does something different--calling out to ICU rather than
our own, custom case conversion ("unibrow").

[jwolfe, 2017-03-14 23:48:15]

The CQ bit was checked by jwolfe@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 23:48:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 00:03:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 00:03:54]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[jwolfe, 2017-03-15 21:49:47]

The CQ bit was checked by jwolfe@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 21:49:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 22:15:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 22:15:55]

Dry run: This issue passed the CQ dry run.

[jwolfe, 2017-03-15 23:10:49]

Description was changed from

==========
[WIP] Migrate case conversion functions from JS to CPP builtins

BUG=v8:5751
==========

to

==========
[WIP] Migrate case conversion functions from JS to CPP builtins

BUG=v8:5751
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng
==========

[jwolfe, 2017-03-15 23:11:11]

The CQ bit was checked by jwolfe@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 23:11:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 23:16:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 23:16:52]

Dry run: Try jobs failed on following builders:
  v8_linux_noi18n_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_noi18n_rel_ng/buil...)

[caitp, 2017-03-15 23:27:12]

caitp@igalia.com changed reviewers:
+ caitp@igalia.com

[caitp, 2017-03-15 23:27:13]

https://codereview.chromium.org/2728763006/diff/60001/src/builtins/builtins-i...
File src/builtins/builtins-intl.cc (right):

https://codereview.chromium.org/2728763006/diff/60001/src/builtins/builtins-i...
src/builtins/builtins-intl.cc:8: #include "src/i18n.h"
I think you need to wrap this whole file in `#ifdef V8_I18N_SUPPORT` to avoid
the error in noi18n builds (similar to runtime-i18n.cc)

[caitp, 2017-03-15 23:28:38]

https://codereview.chromium.org/2728763006/diff/60001/src/builtins/builtins-i...
File src/builtins/builtins-intl.cc (right):

https://codereview.chromium.org/2728763006/diff/60001/src/builtins/builtins-i...
src/builtins/builtins-intl.cc:8: #include "src/i18n.h"
On 2017/03/15 23:27:12, caitp wrote:
> I think you need to wrap this whole file in `#ifdef V8_I18N_SUPPORT` to avoid
> the error in noi18n builds (similar to runtime-i18n.cc)

ah, having re-read your IRC messages I see you already knew this. Carry on!

[jwolfe, 2017-03-16 00:12:10]

The CQ bit was checked by jwolfe@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 00:12:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 00:39:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 00:39:22]

Dry run: This issue passed the CQ dry run.

[jwolfe, 2017-03-17 22:05:13]

Description was changed from

==========
[WIP] Migrate case conversion functions from JS to CPP builtins

BUG=v8:5751
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng
==========

to

==========
Migrate String.prototype.to{Upper,Lower}Case functions from JS to CPP builtins.

Move ICU case conversion utility functions to a common location.

BUG=v8:5751
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng
==========

[jwolfe, 2017-03-17 22:19:21]

PTAL.

Sorry, I got a rebase tangled up in the latest patch set. The only change I made
was removing the i18n logic from BUILD.gn and adding an #ifdef V8_I18N_SUPPORT
to builtins-intl.cc. I decided to go with an #ifdef instead of build
configuration logic because my first try to get the logic to work in src/v8.gyp
didn't work.

I've got a TODO and some commented out code in builtins.h that might be a YAGNI
violation. I'm happy to remove that if that would be appropriate.

[Jakob Kummerow, 2017-03-18 12:27:33]

DBC.

https://codereview.chromium.org/2728763006/diff/100001/src/builtins/builtins-...
File src/builtins/builtins-intl.cc (right):

https://codereview.chromium.org/2728763006/diff/100001/src/builtins/builtins-...
src/builtins/builtins-intl.cc:9: #include "src/code-stub-assembler.h"
This doesn't need the CodeStubAssembler. However, you might want to include
objects-inl.h instead (that's where e.g. String::Flatten lives).

[jwolfe, 2017-03-20 23:47:37]

On 2017/03/18 12:27:33, Jakob Kummerow wrote:
> DBC.

Pardon my ignorance. Does DBC stand for "didn't bother checking" or "don't
bother correcting" or something else? I wasn't able to determine what that meant
with any confidence just by googling.

[Dan Ehrenberg, 2017-03-21 09:30:59]

On 2017/03/20 23:47:37, jwolfe wrote:
> On 2017/03/18 12:27:33, Jakob Kummerow wrote:
> > DBC.
> 
> Pardon my ignorance. Does DBC stand for "didn't bother checking" or "don't
> bother correcting" or something else? I wasn't able to determine what that
meant
> with any confidence just by googling.

Probably "Drive-By Codereview".

[Dan Ehrenberg, 2017-03-21 09:31:17]

lgtm

LGTM modulo a nit and Jakob's comment.

https://codereview.chromium.org/2728763006/diff/120001/src/builtins/builtins.h
File src/builtins/builtins.h (right):

https://codereview.chromium.org/2728763006/diff/120001/src/builtins/builtins....
src/builtins/builtins.h:884: /* CPP(StringPrototypeToLocaleUpperCaseI18N) */    
           \
Nit: Could you not check in these lines? I think this sort of thing would be
better handled in the bug tracker, or in a single-line TODO rather than all this
commented-out code.

[Dan Ehrenberg, 2017-03-21 09:31:59]

On 2017/03/21 09:31:17, Dan Ehrenberg wrote:
> lgtm
> 
> LGTM modulo a nit and Jakob's comment.

Never mind, I see you've already fixed that well.

[jwolfe, 2017-03-21 21:52:06]

The CQ bit was checked by jwolfe@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 21:52:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 22:17:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 22:17:16]

Dry run: This issue passed the CQ dry run.

[jwolfe, 2017-03-21 22:22:01]

The CQ bit was checked by jwolfe@igalia.com

[jwolfe, 2017-03-21 22:22:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/2728763006/#ps140001
(title: "remove TODO comment")

[commit-bot: I haz the power, 2017-03-21 22:22:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 22:23:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 22:23:26]

Failed to apply patch for src/builtins/builtins.h:
While running git apply --index -p1;
  error: patch failed: src/builtins/builtins.h:876
  error: src/builtins/builtins.h: patch does not apply

Patch:       src/builtins/builtins.h
Index: src/builtins/builtins.h
diff --git a/src/builtins/builtins.h b/src/builtins/builtins.h
index
2137129d759a0d302e2ab73ac57f9ea357238505..ba5a0664a70ba9de6d2080f15ad8880086444334
100644
--- a/src/builtins/builtins.h
+++ b/src/builtins/builtins.h
@@ -51,7 +51,7 @@ class Isolate;
 // DBG: Builtin in platform-dependent assembly, used by the debugger.
 //      Args: name
 
-#define BUILTIN_LIST(CPP, API, TFJ, TFS, ASM, ASH, DBG)                       
\
+#define BUILTIN_LIST_BASE(CPP, API, TFJ, TFS, ASM, ASH, DBG)                  
\
   ASM(Abort)                                                                  
\
   /* Code aging */                                                            
\
   CODE_AGE_LIST_WITH_ARG(DECLARE_CODE_AGE_BUILTIN, ASM)                       
\
@@ -795,9 +795,9 @@ class Isolate;
   CPP(StringPrototypeToLocaleLowerCase)                                       
\
   /* ES #sec-string.prototype.tolocaleuppercase */                            
\
   CPP(StringPrototypeToLocaleUpperCase)                                       
\
-  /* ES #sec-string.prototype.tolowercase */                                  
\
+  /* (obsolete) Unibrow version */                                            
\
   CPP(StringPrototypeToLowerCase)                                             
\
-  /* ES #sec-string.prototype.touppercase */                                  
\
+  /* (obsolete) Unibrow version */                                            
\
   CPP(StringPrototypeToUpperCase)                                             
\
   CPP(StringPrototypeTrim)                                                    
\
   CPP(StringPrototypeTrimLeft)                                                
\
@@ -876,6 +876,19 @@ class Isolate;
   /* proposal-async-iteration/#sec-async-iterator-value-unwrap-functions */   
\
   TFJ(AsyncIteratorValueUnwrap, 1)
 
+#ifdef V8_I18N_SUPPORT
+#define BUILTIN_LIST(CPP, API, TFJ, TFS, ASM, ASH, DBG) \
+  BUILTIN_LIST_BASE(CPP, API, TFJ, TFS, ASM, ASH, DBG)  \
+                                                        \
+  /* ES #sec-string.prototype.tolowercase */            \
+  CPP(StringPrototypeToLowerCaseI18N)                   \
+  /* ES #sec-string.prototype.touppercase */            \
+  CPP(StringPrototypeToUpperCaseI18N)
+#else
+#define BUILTIN_LIST(CPP, API, TFJ, TFS, ASM, ASH, DBG) \
+  BUILTIN_LIST_BASE(CPP, API, TFJ, TFS, ASM, ASH, DBG)
+#endif  // V8_I18N_SUPPORT
+
 #define BUILTIN_PROMISE_REJECTION_PREDICTION_LIST(V) \
   V(AsyncFromSyncIteratorPrototypeNext)              \
   V(AsyncFromSyncIteratorPrototypeReturn)            \

[jwolfe, 2017-03-22 22:34:20]

The CQ bit was checked by jwolfe@igalia.com

[jwolfe, 2017-03-22 22:34:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/2728763006/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-22 22:34:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 23:06:37]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490222060167040,
"parent_rev": "ff1a155a22f8b4a10e9b1b98dee18f01760d6cb4", "commit_rev":
"4a5d1e253573d98fb368f5575412d62e07ee1cf3"}

[commit-bot: I haz the power, 2017-03-22 23:06:43]

Description was changed from

==========
Migrate String.prototype.to{Upper,Lower}Case functions from JS to CPP builtins.

Move ICU case conversion utility functions to a common location.

BUG=v8:5751
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng
==========

to

==========
Migrate String.prototype.to{Upper,Lower}Case functions from JS to CPP builtins.

Move ICU case conversion utility functions to a common location.

BUG=v8:5751
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng

Review-Url: https://codereview.chromium.org/2728763006
Cr-Commit-Position: refs/heads/master@{#44050}
Committed:
https://chromium.googlesource.com/v8/v8/+/4a5d1e253573d98fb368f5575412d62e07e...
==========

[commit-bot: I haz the power, 2017-03-22 23:06:44]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/v8/v8/+/4a5d1e253573d98fb368f5575412d62e07e...