Register all ExternalReferences in serializer code.

Register all ExternalReferences in serializer code.

[Slava Chigrin, 2014-07-31 16:28:44]

https://codereview.chromium.org/430213004/diff/1/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/430213004/diff/1/src/serialize.cc#newcode530
src/serialize.cc:530:
Add(ExternalReference::flush_icache_function(isolate).address(),
Comment in the start of this function says that "The following populates all of
the different type of external references into the ExternalReferenceTable." But
seems these external references were missed. Even if current built-in snapshot
does not use them, may be worth to add them to table to avoid problems in
future?

https://codereview.chromium.org/430213004/diff/1/src/serialize.cc#newcode546
src/serialize.cc:546: Add(ExternalReference::address_of_zero().address(),
Seems address_of_zero(), address_of_uint8_max_value() and
heap_always_allocate_scope_depth() external references not used at all in
codebase now. What do you think is better - add them to table since they can be
used in future, or just remove them from ExternalReference class?

https://codereview.chromium.org/430213004/diff/1/src/serialize.cc#newcode574
src/serialize.cc:574: 
There are also few external references generated in math_exp_constants()
function. I am going to submit their registration in separate CL. Or I can add
them to this CL, which is better?

[Yang, 2014-08-05 11:02:44]

Sorry for the delay. This CL makes perfect sense, though I do have some
comments.

https://codereview.chromium.org/430213004/diff/1/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/430213004/diff/1/src/serialize.cc#newcode530
src/serialize.cc:530:
Add(ExternalReference::flush_icache_function(isolate).address(),
On 2014/07/31 16:28:44, Slava Chigrin wrote:
> Comment in the start of this function says that "The following populates all
of
> the different type of external references into the ExternalReferenceTable."
But
> seems these external references were missed. Even if current built-in snapshot
> does not use them, may be worth to add them to table to avoid problems in
> future?

Acknowledged.

https://codereview.chromium.org/430213004/diff/1/src/serialize.cc#newcode546
src/serialize.cc:546: Add(ExternalReference::address_of_zero().address(),
On 2014/07/31 16:28:44, Slava Chigrin wrote:
> Seems address_of_zero(), address_of_uint8_max_value() and
> heap_always_allocate_scope_depth() external references not used at all in
> codebase now. What do you think is better - add them to table since they can
be
> used in future, or just remove them from ExternalReference class?

Removing them entirely makes more sense. This also goes for
address_of_minus_zero().

https://codereview.chromium.org/430213004/diff/1/src/serialize.cc#newcode574
src/serialize.cc:574: 
On 2014/07/31 16:28:44, Slava Chigrin wrote:
> There are also few external references generated in math_exp_constants()
> function. I am going to submit their registration in separate CL. Or I can add
> them to this CL, which is better?

There is no need to include the math_exp_* references, as they are always
created on-demand. I would prefer not to invest in unnecessary complexity here.
Please also remove math_exp_log_table.

To better catch bugs in the future, we could add a
  DCHECK(!masm->serializer_enabled());
to EmitMathExp with a comment stating that we do not expect this code to end up
being serialized.

[Slava Chigrin, 2014-08-05 11:47:15]

Thank you so much for review! I tried to fix issues you found in new patch set.

https://codereview.chromium.org/430213004/diff/20001/src/serialize.cc
File src/serialize.cc (left):

https://codereview.chromium.org/430213004/diff/20001/src/serialize.cc#oldcode403
src/serialize.cc:403: 39,
I decided to not renumber all IDs after this, to avoid too big diff. Seems,
somebody already did the same (see IDs jump from 29 to 31 on line 371). I can
perform renumbering, if you think it is worth it.

[Yang, 2014-08-05 12:53:00]

On 2014/08/05 11:47:15, Slava Chigrin wrote:
> Thank you so much for review! I tried to fix issues you found in new patch
set.
> 
> https://codereview.chromium.org/430213004/diff/20001/src/serialize.cc
> File src/serialize.cc (left):
> 
>
https://codereview.chromium.org/430213004/diff/20001/src/serialize.cc#oldcode403
> src/serialize.cc:403: 39,
> I decided to not renumber all IDs after this, to avoid too big diff. Seems,
> somebody already did the same (see IDs jump from 29 to 31 on line 371). I can
> perform renumbering, if you think it is worth it.

I think registering external references in the serializer is worth policing, so
I created this: https://codereview.chromium.org/441983002/

[Slava Chigrin, 2014-08-05 13:08:11]

On 2014/08/05 12:53:00, Yang wrote:
> On 2014/08/05 11:47:15, Slava Chigrin wrote:
> > Thank you so much for review! I tried to fix issues you found in new patch
> set.
> > 
> > https://codereview.chromium.org/430213004/diff/20001/src/serialize.cc
> > File src/serialize.cc (left):
> > 
> >
>
https://codereview.chromium.org/430213004/diff/20001/src/serialize.cc#oldcode403
> > src/serialize.cc:403: 39,
> > I decided to not renumber all IDs after this, to avoid too big diff. Seems,
> > somebody already did the same (see IDs jump from 29 to 31 on line 371). I
can
> > perform renumbering, if you think it is worth it.
> 
> I think registering external references in the serializer is worth policing,
so
> I created this: https://codereview.chromium.org/441983002/

I checked your issue, it seems much more comprehensive then mine. I like idea of
having script for automatic checking ExternalReferences registration. Thank you
so much!