Extend assembler with ability to produce comments for the generated code.

Extend assembler with ability to produce comments for the generated code.

Extend x64 disassembler with ability to decode and output recorded comments.

Example output (currently comments are emitted only by flow graph compiler at block/instruction boundaries, once instruction printing is refactored and supports printing into buffer simple "instruction" comment will be replaced with the actual IL construct):

        ;; B0
        ;; B1
        ;; instruction
0x007f0fff812525 00000055 49 bb 21 00 b4 04 10  mov    $0x7f1004b40021,%r11
0x007f0fff81252c 0000005c 7f 00 00 
0x007f0fff81252f 0000005f 41 53                 push   %r11

R=srdjan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=7723

[Vyacheslav Egorov (Google), 2012-05-17 15:58:13]

+iposva

[srdjan, 2012-05-17 17:22:08]

First set of comments

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_ia32.cc#n...
runtime/vm/assembler_ia32.cc:17: DEFINE_FLAG(bool, code_comments, false,
"Include comments into disassembly");
and code.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_ia32.cc#n...
runtime/vm/assembler_ia32.cc:1573: CodeComments comments =
CodeComments::New(comments_.length());
Make CodeComments ZoneObject and return "const CodeComments&" (not modifiable).

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_ia32.cc#n...
runtime/vm/assembler_ia32.cc:1575: for (int i = 0; i < comments_.length(); i++)
{
intptr_t

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_ia32.h
File runtime/vm/assembler_ia32.h (right):

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_ia32.h#ne...
runtime/vm/assembler_ia32.h:282: Assembler() : buffer_(), prolog_offset_(-1) { }
, comments_()

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_ia32.h#ne...
runtime/vm/assembler_ia32.h:565: CodeComments GetComments();
CodeComments is too close to CodeComment (confusing). Can be solved by moving
CodeComments into Code, thus returning Code::Comments.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_ia32.h#ne...
runtime/vm/assembler_ia32.h:565: CodeComments GetComments();
GetComments -> GetCodeComments since you are not returning comments_. Can it be
const?

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_ia32.h#ne...
runtime/vm/assembler_ia32.h:581: const String& comment_;
Add DISALLOW_bla_bla

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_x64.h#new...
runtime/vm/assembler_x64.h:534: CodeComments GetComments();
ditto from ia32

http://codereview.chromium.org/10407019/diff/1/runtime/vm/disassembler.h
File runtime/vm/disassembler.h (right):

http://codereview.chromium.org/10407019/diff/1/runtime/vm/disassembler.h#newc...
runtime/vm/disassembler.h:66: CodeComments comments);
const CodeComments& comments

http://codereview.chromium.org/10407019/diff/1/runtime/vm/disassembler_ia32.cc
File runtime/vm/disassembler_ia32.cc (right):

http://codereview.chromium.org/10407019/diff/1/runtime/vm/disassembler_ia32.c...
runtime/vm/disassembler_ia32.cc:1581: CodeComments comments) {
const CodeComments&

http://codereview.chromium.org/10407019/diff/1/runtime/vm/disassembler_x64.cc
File runtime/vm/disassembler_x64.cc (right):

http://codereview.chromium.org/10407019/diff/1/runtime/vm/disassembler_x64.cc...
runtime/vm/disassembler_x64.cc:23: CodeComments comments) {
const CodeComments&

http://codereview.chromium.org/10407019/diff/1/runtime/vm/flow_graph_compiler...
File runtime/vm/flow_graph_compiler_x64.cc (right):

http://codereview.chromium.org/10407019/diff/1/runtime/vm/flow_graph_compiler...
runtime/vm/flow_graph_compiler_x64.cc:1131: __ Comment("instruction");
Maybe also emit the instruction's cid (unique id, to be used for deoptimization,
etc).

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2146
runtime/vm/object.h:2146: class CodeComments : public ValueObject {
Please move CodeComments inside class Code.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2152
runtime/vm/object.h:2152: };
name the enum, and why do you need it?

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2152
runtime/vm/object.h:2152: };
name the enum, and why do you need it?

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2154
runtime/vm/object.h:2154: static CodeComments New(int count);
Memory sizes are in intptr_t.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2158
runtime/vm/object.h:2158: intptr_t PCAt(int idx);
Is it PC or PCOffset?

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2159
runtime/vm/object.h:2159: void SetPCAt(int idx, intptr_t pc);
pc_offset?

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2161
runtime/vm/object.h:2161: const String& CommentAt(int idx);
const?

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2166
runtime/vm/object.h:2166: CodeComments();
Remove this constructor once it becomes ZoneObject

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2170
runtime/vm/object.h:2170: friend class Code;
Remove this once moved inside class Code, I guess.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2211
runtime/vm/object.h:2211: CodeComments comments() const;
const CodeComments&

[Vyacheslav Egorov (Google), 2012-05-17 21:16:27]

comments addressed

please take another look

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_ia32.cc#n...
runtime/vm/assembler_ia32.cc:17: DEFINE_FLAG(bool, code_comments, false,
"Include comments into disassembly");
On 2012/05/17 17:22:08, srdjan wrote:
> and code.

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_ia32.cc#n...
runtime/vm/assembler_ia32.cc:1573: CodeComments comments =
CodeComments::New(comments_.length());
On 2012/05/17 17:22:08, srdjan wrote:
> Make CodeComments ZoneObject and return "const CodeComments&" (not
modifiable).

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_ia32.cc#n...
runtime/vm/assembler_ia32.cc:1575: for (int i = 0; i < comments_.length(); i++)
{
On 2012/05/17 17:22:08, srdjan wrote:
> intptr_t

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_ia32.h
File runtime/vm/assembler_ia32.h (right):

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_ia32.h#ne...
runtime/vm/assembler_ia32.h:282: Assembler() : buffer_(), prolog_offset_(-1) { }
On 2012/05/17 17:22:08, srdjan wrote:
> , comments_()

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_ia32.h#ne...
runtime/vm/assembler_ia32.h:565: CodeComments GetComments();
On 2012/05/17 17:22:08, srdjan wrote:
> GetComments -> GetCodeComments since you are not returning comments_. Can it
be
> const?

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_ia32.h#ne...
runtime/vm/assembler_ia32.h:581: const String& comment_;
On 2012/05/17 17:22:08, srdjan wrote:
> Add DISALLOW_bla_bla

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

http://codereview.chromium.org/10407019/diff/1/runtime/vm/assembler_x64.h#new...
runtime/vm/assembler_x64.h:534: CodeComments GetComments();
On 2012/05/17 17:22:08, srdjan wrote:
> ditto from ia32

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/disassembler.h
File runtime/vm/disassembler.h (right):

http://codereview.chromium.org/10407019/diff/1/runtime/vm/disassembler.h#newc...
runtime/vm/disassembler.h:66: CodeComments comments);
On 2012/05/17 17:22:08, srdjan wrote:
> const CodeComments& comments

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/disassembler_ia32.cc
File runtime/vm/disassembler_ia32.cc (right):

http://codereview.chromium.org/10407019/diff/1/runtime/vm/disassembler_ia32.c...
runtime/vm/disassembler_ia32.cc:1581: CodeComments comments) {
On 2012/05/17 17:22:08, srdjan wrote:
> const CodeComments&

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/disassembler_x64.cc
File runtime/vm/disassembler_x64.cc (right):

http://codereview.chromium.org/10407019/diff/1/runtime/vm/disassembler_x64.cc...
runtime/vm/disassembler_x64.cc:23: CodeComments comments) {
On 2012/05/17 17:22:08, srdjan wrote:
> const CodeComments&

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/flow_graph_compiler...
File runtime/vm/flow_graph_compiler_x64.cc (right):

http://codereview.chromium.org/10407019/diff/1/runtime/vm/flow_graph_compiler...
runtime/vm/flow_graph_compiler_x64.cc:1131: __ Comment("instruction");
On 2012/05/17 17:22:08, srdjan wrote:
> Maybe also emit the instruction's cid (unique id, to be used for
deoptimization,
> etc).

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2146
runtime/vm/object.h:2146: class CodeComments : public ValueObject {
On 2012/05/17 17:22:08, srdjan wrote:
> Please move CodeComments inside class Code.

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2152
runtime/vm/object.h:2152: };
On 2012/05/17 17:22:08, srdjan wrote:
> name the enum, and why do you need it?

Discussed offline. Moved enum to private section. Added comments.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2154
runtime/vm/object.h:2154: static CodeComments New(int count);
On 2012/05/17 17:22:08, srdjan wrote:
> Memory sizes are in intptr_t.

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2158
runtime/vm/object.h:2158: intptr_t PCAt(int idx);
On 2012/05/17 17:22:08, srdjan wrote:
> Is it PC or PCOffset?

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2159
runtime/vm/object.h:2159: void SetPCAt(int idx, intptr_t pc);
On 2012/05/17 17:22:08, srdjan wrote:
> pc_offset?

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2161
runtime/vm/object.h:2161: const String& CommentAt(int idx);
On 2012/05/17 17:22:08, srdjan wrote:
> const?

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2161
runtime/vm/object.h:2161: const String& CommentAt(int idx);
On 2012/05/17 17:22:08, srdjan wrote:
> const?

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2166
runtime/vm/object.h:2166: CodeComments();
On 2012/05/17 17:22:08, srdjan wrote:
> Remove this constructor once it becomes ZoneObject

Done.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2170
runtime/vm/object.h:2170: friend class Code;
On 2012/05/17 17:22:08, srdjan wrote:
> Remove this once moved inside class Code, I guess.

Moving one class into another does not make these classes friends automatically.

http://codereview.chromium.org/10407019/diff/1/runtime/vm/object.h#newcode2211
runtime/vm/object.h:2211: CodeComments comments() const;
On 2012/05/17 17:22:08, srdjan wrote:
> const CodeComments&

Done. However both Code::Comments::New and Code::comments started to look mighty
ugly in my opinion.

[srdjan, 2012-05-17 21:30:37]

LGTM

https://chromiumcodereview.appspot.com/10407019/diff/5002/runtime/vm/disassem...
File runtime/vm/disassembler_ia32.cc (right):

https://chromiumcodereview.appspot.com/10407019/diff/5002/runtime/vm/disassem...
runtime/vm/disassembler_ia32.cc:1582: // TODO(vegorov) Decode and display
comments.
Typically: "TODO(vegorov):" (add colon).

https://chromiumcodereview.appspot.com/10407019/diff/5002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/10407019/diff/5002/runtime/vm/object.c...
runtime/vm/object.cc:6078: comments = new Comments(Array::Handle());
The other option would be to use Array::Empty() in this casem so that you do not
need to check fort NULL-ness. Your choice.

[Ivan Posva, 2012-05-22 22:40:02]

https://chromiumcodereview.appspot.com/10407019/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://chromiumcodereview.appspot.com/10407019/diff/1/runtime/vm/object.h#ne...
runtime/vm/object.h:2211: CodeComments comments() const;
On 2012/05/17 21:16:27, Vyacheslav Egorov (Google) wrote:
> On 2012/05/17 17:22:08, srdjan wrote:
> > const CodeComments&
> 
> Done. However both Code::Comments::New and Code::comments started to look
mighty
> ugly in my opinion.
> 

Maybe the real solution should be that the code comment handling does not have
anything to do with objects (except that the data is stored in an ObjectArray).
In reality code comments are something that is used to communicate between the
assembler and disassembler. How about moving it there?