A64: Remove an invalid assertion about the position of the reloc_info_writer.

A64: Remove an invalid assertion about the position of the reloc_info_writer.

Committed: https://code.google.com/p/v8/source/detail?r=19887

[Alexandre Rames, 2014-03-12 18:40:13]

Fixes the specific test. Running sanity checks...

[Alexandre Rames, 2014-03-12 18:59:25]

On 2014/03/12 18:40:13, Alexandre Rames wrote:
> Fixes the specific test. Running sanity checks...

More details: fixes tools/run-tests.py --arch-and-mode=a64.debug
mozilla/ecma/Date/15.9.5.23-11

Other tests look ok
$ make a64.optdebug.check -j15
[15:12|% 100|+ 12123|-   0]: Done

The check was failing because the branch over the pool can generate a reloc_info
for the code position. 
Can you confirm that the reloc_info_writer does not need to record the reloc 'in
order'? The check assumed it could not, but I actually see no reason it cannot.
And the veneer code does not care about it. (We take the offset - not the
address - of the position we are interested in, so even if the buffer grows
before the RecordRelocInfo we are fine)

[rmcilroy, 2014-03-12 19:19:52]

On 2014/03/12 18:59:25, Alexandre Rames wrote:
> On 2014/03/12 18:40:13, Alexandre Rames wrote:
> > Fixes the specific test. Running sanity checks...
> 
> More details: fixes tools/run-tests.py --arch-and-mode=a64.debug
> mozilla/ecma/Date/15.9.5.23-11
> 
> Other tests look ok
> $ make a64.optdebug.check -j15
> [15:12|% 100|+ 12123|-   0]: Done
> 
> The check was failing because the branch over the pool can generate a
reloc_info
> for the code position. 
> Can you confirm that the reloc_info_writer does not need to record the reloc
'in
> order'? The check assumed it could not, but I actually see no reason it
cannot.
> And the veneer code does not care about it. (We take the offset - not the
> address - of the position we are interested in, so even if the buffer grows
> before the RecordRelocInfo we are fine)

There is a comment in serializer.cc:1765 which says:
  // This assert will fail if the reloc info gives us the target_address_address
  // locations in a non-ascending order.  Luckily that doesn't happen.
(Note: target_address_address returns the address of the constant pool entry). 
Would this change cause this assertion to fail?

[Alexandre Rames, 2014-03-13 10:03:07]

On 2014/03/12 19:19:52, rmcilroy wrote:
> On 2014/03/12 18:59:25, Alexandre Rames wrote:
> > On 2014/03/12 18:40:13, Alexandre Rames wrote:
> > > Fixes the specific test. Running sanity checks...
> > 
> > More details: fixes tools/run-tests.py --arch-and-mode=a64.debug
> > mozilla/ecma/Date/15.9.5.23-11
> > 
> > Other tests look ok
> > $ make a64.optdebug.check -j15
> > [15:12|% 100|+ 12123|-   0]: Done
> > 
> > The check was failing because the branch over the pool can generate a
> reloc_info
> > for the code position. 
> > Can you confirm that the reloc_info_writer does not need to record the reloc
> 'in
> > order'? The check assumed it could not, but I actually see no reason it
> cannot.
> > And the veneer code does not care about it. (We take the offset - not the
> > address - of the position we are interested in, so even if the buffer grows
> > before the RecordRelocInfo we are fine)
> 
> There is a comment in serializer.cc:1765 which says:
>   // This assert will fail if the reloc info gives us the
target_address_address
>   // locations in a non-ascending order.  Luckily that doesn't happen.
> (Note: target_address_address returns the address of the constant pool entry).

> Would this change cause this assertion to fail?

I am not very familiar with the serializer code, but I think this is ok.
I think the Serializer does not need to visit the constant or veneer pool
declaration RelocInfos (RelocInfo::Visit agrees), so we never reach
OutputRawData with the address for a veneer pool RelocInfo.

I just ran the mozilla tests in optdebug mode and they are ok.
However I think this is worth a comment in RecordVeneerPool.

[rmcilroy, 2014-03-13 10:22:59]

On 2014/03/13 10:03:07, Alexandre Rames wrote:
> On 2014/03/12 19:19:52, rmcilroy wrote:
> > On 2014/03/12 18:59:25, Alexandre Rames wrote:
> > > On 2014/03/12 18:40:13, Alexandre Rames wrote:
> > > > Fixes the specific test. Running sanity checks...
> > > 
> > > More details: fixes tools/run-tests.py --arch-and-mode=a64.debug
> > > mozilla/ecma/Date/15.9.5.23-11
> > > 
> > > Other tests look ok
> > > $ make a64.optdebug.check -j15
> > > [15:12|% 100|+ 12123|-   0]: Done
> > > 
> > > The check was failing because the branch over the pool can generate a
> > reloc_info
> > > for the code position. 
> > > Can you confirm that the reloc_info_writer does not need to record the
reloc
> > 'in
> > > order'? The check assumed it could not, but I actually see no reason it
> > cannot.
> > > And the veneer code does not care about it. (We take the offset - not the
> > > address - of the position we are interested in, so even if the buffer
grows
> > > before the RecordRelocInfo we are fine)
> > 
> > There is a comment in serializer.cc:1765 which says:
> >   // This assert will fail if the reloc info gives us the
> target_address_address
> >   // locations in a non-ascending order.  Luckily that doesn't happen.
> > (Note: target_address_address returns the address of the constant pool
entry).
> 
> > Would this change cause this assertion to fail?
> 
> I am not very familiar with the serializer code, but I think this is ok.
> I think the Serializer does not need to visit the constant or veneer pool
> declaration RelocInfos (RelocInfo::Visit agrees), so we never reach
> OutputRawData with the address for a veneer pool RelocInfo.
> 
> I just ran the mozilla tests in optdebug mode and they are ok.
> However I think this is worth a comment in RecordVeneerPool.

Yes I think you are right. Ok LGTM. Thanks

[Alexandre Rames, 2014-03-13 10:24:59]

On 2014/03/13 10:03:07, Alexandre Rames wrote:
> On 2014/03/12 19:19:52, rmcilroy wrote:
> > On 2014/03/12 18:59:25, Alexandre Rames wrote:
> > > On 2014/03/12 18:40:13, Alexandre Rames wrote:
> > > > Fixes the specific test. Running sanity checks...
> > > 
> > > More details: fixes tools/run-tests.py --arch-and-mode=a64.debug
> > > mozilla/ecma/Date/15.9.5.23-11
> > > 
> > > Other tests look ok
> > > $ make a64.optdebug.check -j15
> > > [15:12|% 100|+ 12123|-   0]: Done
> > > 
> > > The check was failing because the branch over the pool can generate a
> > reloc_info
> > > for the code position. 
> > > Can you confirm that the reloc_info_writer does not need to record the
reloc
> > 'in
> > > order'? The check assumed it could not, but I actually see no reason it
> > cannot.
> > > And the veneer code does not care about it. (We take the offset - not the
> > > address - of the position we are interested in, so even if the buffer
grows
> > > before the RecordRelocInfo we are fine)
> > 
> > There is a comment in serializer.cc:1765 which says:
> >   // This assert will fail if the reloc info gives us the
> target_address_address
> >   // locations in a non-ascending order.  Luckily that doesn't happen.
> > (Note: target_address_address returns the address of the constant pool
entry).
> 
> > Would this change cause this assertion to fail?
> 
> I am not very familiar with the serializer code, but I think this is ok.
> I think the Serializer does not need to visit the constant or veneer pool
> declaration RelocInfos (RelocInfo::Visit agrees), so we never reach
> OutputRawData with the address for a veneer pool RelocInfo.
> 
> I just ran the mozilla tests in optdebug mode and they are ok.
> However I think this is worth a comment in RecordVeneerPool.

More thinking on this. There is a danger in
RedirectActivationsToRecompiledCodeOnThread().
I think there is a danger in RedirectActivationsToRecompiledCodeOnThread(),
which does assume an ascending order for the RelocInfos. Currently it is ok
because RelocInfos for DEBUG_BREAK_SLOT, CONST_POOL, and, VENEER_POOL are in
ascending order relatively to each other. But maybe it would be safer to stick
to the ordering assumption.
The issue is that computing the size of the veneer pool ahead of generation is
not very nice. I put that on my TODO list to work on improving this soon.

[Alexandre Rames, 2014-03-13 10:28:59]

Committed patchset #1 manually as r19887 (presubmit successful).