Emit assembly instead of a blob for the instructions snapshot, and provide labels for the entry poi…

Emit assembly instead of a blob for the instructions snapshot, and provide labels for the entry point of each Instructions.

R=asiva@google.com

Committed: https://github.com/dart-lang/sdk/commit/83585cfbf12a416f99fffbbdaacd856f0e706f13

[rmacnak, 2015-09-03 20:17:53]

rmacnak@google.com changed reviewers:
+ asiva@google.com

[rmacnak, 2015-09-03 20:17:53]

https://codereview.chromium.org/1309563006/diff/40001/runtime/vm/snapshot.h
File runtime/vm/snapshot.h (right):

https://codereview.chromium.org/1309563006/diff/40001/runtime/vm/snapshot.h#n...
runtime/vm/snapshot.h:794: void SetInstructionsCode(RawInstructions* insns,
RawCode* code) {
Must not use the back pointer in Instructions because it goes away with the
calling convention change.

[siva, 2015-09-04 20:34:04]

LGTM with one question about alignment.

https://codereview.chromium.org/1309563006/diff/40001/runtime/vm/datastream.h
File runtime/vm/datastream.h (right):

https://codereview.chromium.org/1309563006/diff/40001/runtime/vm/datastream.h...
runtime/vm/datastream.h:455: current_ += len;  // Not + 1 to swallow the
terminating NUL.
Not len + 1 to ...

https://codereview.chromium.org/1309563006/diff/40001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/1309563006/diff/40001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:1152: if (c >= '0' && c <= '9') continue;
why not :

if (((c >= 'a') && (c <= 'z')) ||
    ((c >= 'A' ........     )) ||
    ((c >= '0' ..........   ))) {
  continue;
}

https://codereview.chromium.org/1309563006/diff/40001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:1161: // Handlify collected raw pointers as the building
the names below
as building the names

https://codereview.chromium.org/1309563006/diff/40001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:1180: const Code& code = *instructions_[i].code_;
Why do you have that additional loop on top to create handles why not just do
that in this loop here:
  InstructionsData& data = instructions_[i];
  data.insns_= &Instructions::Handle(Z, data.raw_insns_);
  const Instructions& insns = *data.insns_;
  data.code_ = ...;
  const Code& code = ...;

https://codereview.chromium.org/1309563006/diff/40001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:1227: ASSERT(Utils::IsAligned(end, sizeof(uint64_t)));
are these assertions valid for all architectures including ia32?

[rmacnak, 2015-09-04 22:11:57]

https://codereview.chromium.org/1309563006/diff/40001/runtime/vm/datastream.h
File runtime/vm/datastream.h (right):

https://codereview.chromium.org/1309563006/diff/40001/runtime/vm/datastream.h...
runtime/vm/datastream.h:455: current_ += len;  // Not + 1 to swallow the
terminating NUL.
On 2015/09/04 20:34:04, siva wrote:
> Not len + 1 to ...

Done.

https://codereview.chromium.org/1309563006/diff/40001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/1309563006/diff/40001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:1152: if (c >= '0' && c <= '9') continue;
On 2015/09/04 20:34:04, siva wrote:
> why not :
> 
> if (((c >= 'a') && (c <= 'z')) ||
>     ((c >= 'A' ........     )) ||
>     ((c >= '0' ..........   ))) {
>   continue;
> }

Done.

https://codereview.chromium.org/1309563006/diff/40001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:1161: // Handlify collected raw pointers as the building
the names below
On 2015/09/04 20:34:04, siva wrote:
> as building the names

Done.

https://codereview.chromium.org/1309563006/diff/40001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:1180: const Code& code = *instructions_[i].code_;
On 2015/09/04 20:34:04, siva wrote:
> Why do you have that additional loop on top to create handles why not just do
> that in this loop here:
>   InstructionsData& data = instructions_[i];
>   data.insns_= &Instructions::Handle(Z, data.raw_insns_);
>   const Instructions& insns = *data.insns_;
>   data.code_ = ...;
>   const Code& code = ...;

As discussed offline, GC can happen in the middle of this loop.

https://codereview.chromium.org/1309563006/diff/40001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:1227: ASSERT(Utils::IsAligned(end, sizeof(uint64_t)));
On 2015/09/04 20:34:04, siva wrote:
> are these assertions valid for all architectures including ia32?

The minimum OS::PreferredCodeAlignment() is 16.

[rmacnak, 2015-09-04 22:15:55]

Committed patchset #4 (id:60001) manually as
83585cfbf12a416f99fffbbdaacd856f0e706f13 (presubmit successful).