Add metadata to order files

Add metadata to order files

This CL adds metadata to order files produced by reorder.exe. In turn, relink.exe uses this metadata to ensure that it is being applied to the appropriate input module.


BUG=http://code.google.com/p/sawbuck/issues/detail?id=37
Committed: http://code.google.com/p/sawbuck/source/browse/#svn/trunk377

[chrisha, 2011-07-06 18:46:53]

PTAL

[Siggi, 2011-07-06 19:39:07]

I have but a couple of nits, though string escaping problems are super annoying.

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc
File syzygy/reorder/reorderer.cc (right):

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
syzygy/reorder/reorderer.cc:194: return OutputText(file, "{", 0, pretty_print)
&&
nit: maybe it's "simpler" here to build the dict as a value and output it
through the existing JSON serialization. This should take care of string
escaping, for one thing.

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
syzygy/reorder/reorderer.cc:750: if (!OutputComment(file, comment.c_str(), 0,
true) ||
a little bit of running commentary might help here.

[Siggi, 2011-07-06 19:42:36]

Ah - you DO escape the file path. In that case:

lgtm.

On 2011/07/06 19:39:07, Siggi wrote:
> I have but a couple of nits, though string escaping problems are super
annoying.
> 
> http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc
> File syzygy/reorder/reorderer.cc (right):
> 
>
http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
> syzygy/reorder/reorderer.cc:194: return OutputText(file, "{", 0, pretty_print)
> &&
> nit: maybe it's "simpler" here to build the dict as a value and output it
> through the existing JSON serialization. This should take care of string
> escaping, for one thing.
> 
>
http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
> syzygy/reorder/reorderer.cc:750: if (!OutputComment(file, comment.c_str(), 0,
> true) ||
> a little bit of running commentary might help here.

[Roger McFarlane, 2011-07-06 19:45:38]

lgtm with nits

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc
File syzygy/reorder/reorderer.cc (right):

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
syzygy/reorder/reorderer.cc:89: !OutputKey(file, "section_id", indent + 2,
pretty_print) ||
Add section_name here as well?

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
syzygy/reorder/reorderer.cc:761: if (!OutputKey(file, "syzygy_version", 4,
pretty_print) ||
Can be folded into previous 'if' (moving the signature retrieval before the if).

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
syzygy/reorder/reorderer.cc:770: if (!OutputText(file, "},", 2, pretty_print) ||
Can be folded into previous 'if'

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
syzygy/reorder/reorderer.cc:775: if (!OutputKey(file, "sections", 2,
pretty_print) ||
Can be folded into previous 'if'

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
syzygy/reorder/reorderer.cc:794: comment = StringPrintf("section_name =
\"%s\".", comment.c_str());
maybe a part of the data in the file?

As in:

  "sections" : [
      { "id" : 0, "name" : ".foo", "blocks" : [0. 1, 2, 3, 4] },
      { "id" : 1, "name" : ".bar", "blocks" : [5, 6, 7, 8, 9] }
    ]

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
syzygy/reorder/reorderer.cc:914: LOG(ERROR) << "Order file does not contain a
valid JSON dictionary.";
return false;

[chrisha, 2011-07-06 20:04:00]

Addressed everyone's comments. rogerm@, PTAL?

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc
File syzygy/reorder/reorderer.cc (right):

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
syzygy/reorder/reorderer.cc:89: !OutputKey(file, "section_id", indent + 2,
pretty_print) ||
On 2011/07/06 19:45:38, Roger McFarlane wrote:
> Add section_name here as well?

We could, but it's effectively redundant. I output it as a comment just for
added readability of the file.

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
syzygy/reorder/reorderer.cc:194: return OutputText(file, "{", 0, pretty_print)
&&
On 2011/07/06 19:39:08, Siggi wrote:
> nit: maybe it's "simpler" here to build the dict as a value and output it
> through the existing JSON serialization. This should take care of string
> escaping, for one thing.

There's no way to output comments in the existing mechanism, and the
pretty-printing we do would be near impossible. As for string escaping, I use
the JSON library provided escaping mechanism.

I have a stream-like formatted JSON output class that I created once upon a time
(when looking into serializing DecomposedImage without creating an intermediate
Value representation). It'd likely be eminently more readable to resurrect this
and use it here.

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
syzygy/reorder/reorderer.cc:750: if (!OutputComment(file, comment.c_str(), 0,
true) ||
On 2011/07/06 19:39:08, Siggi wrote:
> a little bit of running commentary might help here.

Done.

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
syzygy/reorder/reorderer.cc:761: if (!OutputKey(file, "syzygy_version", 4,
pretty_print) ||
On 2011/07/06 19:45:38, Roger McFarlane wrote:
> Can be folded into previous 'if' (moving the signature retrieval before the
if).

I keep them separate more for readability, and the compiler likely folds them
all together anyways.

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
syzygy/reorder/reorderer.cc:770: if (!OutputText(file, "},", 2, pretty_print) ||
On 2011/07/06 19:45:38, Roger McFarlane wrote:
> Can be folded into previous 'if'

Ditto.

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
syzygy/reorder/reorderer.cc:775: if (!OutputKey(file, "sections", 2,
pretty_print) ||
On 2011/07/06 19:45:38, Roger McFarlane wrote:
> Can be folded into previous 'if'

Ditto.

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
syzygy/reorder/reorderer.cc:794: comment = StringPrintf("section_name =
\"%s\".", comment.c_str());
On 2011/07/06 19:45:38, Roger McFarlane wrote:
> maybe a part of the data in the file?
> 
> As in:
> 
>   "sections" : [
>       { "id" : 0, "name" : ".foo", "blocks" : [0. 1, 2, 3, 4] },
>       { "id" : 1, "name" : ".bar", "blocks" : [5, 6, 7, 8, 9] }
>     ]

Could be, but it's redundant. It's also easier in the relinker to refer to
sections by id rather than by name. If you feels strongly about this, we could
still output them, as there's no obligation for LoadFromJSON to actually parse
them.

http://codereview.appspot.com/4672047/diff/3009/syzygy/reorder/reorderer.cc#n...
syzygy/reorder/reorderer.cc:914: LOG(ERROR) << "Order file does not contain a
valid JSON dictionary.";
On 2011/07/06 19:45:38, Roger McFarlane wrote:
> return false;

Done.

[Roger McFarlane, 2011-07-06 20:08:05]

lgtm

[chrisha, 2011-07-06 20:14:57]

Thanks, committing.