Function Layout, Global Variable Layout and Pooled Constants Layout Reordering

Function Layout, Global Variable Layout and Pooled Constants Layout Reordering

PURPOSE:
The purpose of function layout reordering is to defend against code-reuse attacks as the location of code blocks will be various among different binaries. The layout reordering for global variables and pooled constants can be considered as static data randomization. This is to stop memory corruption attacks by randomizing the locations of the static data. After function layout reordering, the order of function blocks in TEXT section will be randomized. Global variable reordering randomize the order of global variables, and pooled constant reordering randomize the order of pooled constants. Note the order of constant pools won’t be affected and all pooled constants will remain in their original constant pools.

USAGE:
-reorder-functions: bool type command line option, enables function layout shuffling in TEXT section. Note when -threads=0 is set, function reordering will be forced off.

-reorder-functions-window-size: uint32 type command line option, specify the length of the shuffling queue. Note -reorder-functions-window-size=0 or 1 means no shuffling applied to functions.

-reorder-global-variables: bool type command line option, enables global variables shuffling.

-reorder-pooled-constants: bool type command line option, enables pooled constants shuffling.

APPROACH:
Randomization is introduced at the code emission time. We use a shuffling method to randomize the emission of function code, global variables and pooled constants. For function code emission, we also introduce “window size” as a parameter to control the size of the function holding buffer for shuffling. Window size 1 and 0 mean no shuffling applied, and a value higher than the number of translated functions means holding all the functions and shuffling them before emitting any of them.

IMPLEMENTATION:
    Function reordering:
        GlobalContext::emitItems(): Call RandomShuffle() routine to shuffle a specific part of the Pending vector.

    Global variable reorder:
        GlobalContext::lowerGlobals(const IceString &SectionSuffix): Call RandomShuffle() routine upon declaration list: Globals.

    Pooled constant reordering:
        TargetDataX8632::emitConstantPool(GlobalContext *Ctx): Add call to RandomShuffle() to shuffle the constant pool to be emitted. This is for asm output.

        ELFObjectWriter::writeConstantPool(Type Tu): Add call to RandomShuffle() to shuffle the constant pool before emitting it. This is only for elf output.

ISSUES:
    The initialization of global variables are emitted along with function code, all of them are considered as EmitterWorkItem. However, we do need to first emit global variables to keep the block profiling workflow untouched. To fulfill this, a “kind” check is added in the while loop of GlobalContext::emitItems(). The “if” statement at line 480 shows the workaround of this issue.
BUG=
R=jpp@chromium.org, jvoung@chromium.org, stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=7cd5351c07806a1bc7ba1c1aec3380f7b715bda8

[qining, 2015-06-24 00:06:07]

qining@google.com changed reviewers:
+ jfb@chromium.org, jpp@chromium.org, jvoung@chromium.org, stichnot@chromium.org

[qining, 2015-06-24 00:09:27]

Changes in IceCompileServer.cpp, IceTargetLoweringX8664.h and
IceTargetLoweringX86BaseImpl.h are introduced by clang-format.

[John, 2015-06-24 00:24:00]

A few comments to get you started (I had to leave as soon as I reached the
GlobalContext changes...)

https://codereview.chromium.org/1206723003/diff/1/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1206723003/diff/1/src/IceClFlags.cpp#newcode336
src/IceClFlags.cpp:336: OutFlags.ReorderFunctions = false;
Move these to line 332 (i.e., keep it sorted alphabetically.)

https://codereview.chromium.org/1206723003/diff/1/src/IceClFlags.cpp#newcode346
src/IceClFlags.cpp:346: OutFlags.RandomizeAndPoolImmediatesOption = RPI_None;
And these, to 344

https://codereview.chromium.org/1206723003/diff/1/src/IceClFlags.h
File src/IceClFlags.h (left):

https://codereview.chromium.org/1206723003/diff/1/src/IceClFlags.h#oldcode82
src/IceClFlags.h:82: 
why was this removed?

https://codereview.chromium.org/1206723003/diff/1/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1206723003/diff/1/src/IceClFlags.h#newcode154
src/IceClFlags.h:154: 
to be consistent with the rest of this file do not skip lines between getters
and setters, but skip a line after each property.

https://codereview.chromium.org/1206723003/diff/1/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1206723003/diff/1/src/IceELFObjectWriter.cpp#...
src/IceELFObjectWriter.cpp:393: const auto Data =
please change this to

const auto& Data,

or this will copy contents.

https://codereview.chromium.org/1206723003/diff/1/src/IceELFObjectWriter.cpp#...
src/IceELFObjectWriter.cpp:514: RandomShuffle(Pool.begin(), Pool.end(),
[this](uint64_t N) {
Can you capture [Ctx] instead of [this]?

https://codereview.chromium.org/1206723003/diff/1/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1206723003/diff/1/src/IceGlobalContext.cpp#ne...
src/IceGlobalContext.cpp:400: [this](int N) { return (uint32_t)RNG.next(N); });
[RNG], instead of [this]?

https://codereview.chromium.org/1206723003/diff/1/src/IceGlobalContext.cpp#ne...
src/IceGlobalContext.cpp:438: uint32_t ShuffleWindowSize =
getFlags().getReorderFunctionsWindowSize() > 0
const uint32_t

[qining, 2015-06-24 01:20:40]

There is problem about capturing the variables for lambda functions. Ctx, and
RNG are both member variables when we capture them, usually we need to capture
'this' to access them and it seems we can't capture them or their reference
directly. So I declared a pointer for each of them and capture the pointer
instead.But I'm not so sure this is better than capturing 'this'.

https://codereview.chromium.org/1206723003/diff/1/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1206723003/diff/1/src/IceClFlags.cpp#newcode336
src/IceClFlags.cpp:336: OutFlags.ReorderFunctions = false;
On 2015/06/24 00:24:00, John wrote:
> Move these to line 332 (i.e., keep it sorted alphabetically.)

Done.

https://codereview.chromium.org/1206723003/diff/1/src/IceClFlags.cpp#newcode346
src/IceClFlags.cpp:346: OutFlags.RandomizeAndPoolImmediatesOption = RPI_None;
On 2015/06/24 00:24:00, John wrote:
> And these, to 344

Done.

https://codereview.chromium.org/1206723003/diff/1/src/IceClFlags.h
File src/IceClFlags.h (left):

https://codereview.chromium.org/1206723003/diff/1/src/IceClFlags.h#oldcode82
src/IceClFlags.h:82: 
On 2015/06/24 00:24:00, John wrote:
> why was this removed?

Done.

https://codereview.chromium.org/1206723003/diff/1/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1206723003/diff/1/src/IceClFlags.h#newcode154
src/IceClFlags.h:154: 
On 2015/06/24 00:24:00, John wrote:
> to be consistent with the rest of this file do not skip lines between getters
> and setters, but skip a line after each property.

Done.

https://codereview.chromium.org/1206723003/diff/1/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1206723003/diff/1/src/IceELFObjectWriter.cpp#...
src/IceELFObjectWriter.cpp:393: const auto Data =
On 2015/06/24 00:24:00, John wrote:
> please change this to
> 
> const auto& Data,
> 
> or this will copy contents.

Done.

https://codereview.chromium.org/1206723003/diff/1/src/IceELFObjectWriter.cpp#...
src/IceELFObjectWriter.cpp:514: RandomShuffle(Pool.begin(), Pool.end(),
[this](uint64_t N) {
On 2015/06/24 00:24:00, John wrote:
> Can you capture [Ctx] instead of [this]?

Done. But it seems I can't capture Ctx or its reference directly as it is a
member of this class. My workaround is declaring a pointer to Ctx and capture
the pointer instead. But actually I'm not sure this is better than capturing
this.

https://codereview.chromium.org/1206723003/diff/1/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1206723003/diff/1/src/IceGlobalContext.cpp#ne...
src/IceGlobalContext.cpp:400: [this](int N) { return (uint32_t)RNG.next(N); });
On 2015/06/24 00:24:00, John wrote:
> [RNG], instead of [this]?

Done. But also add a declaration a pointer to RNG.

https://codereview.chromium.org/1206723003/diff/1/src/IceGlobalContext.cpp#ne...
src/IceGlobalContext.cpp:438: uint32_t ShuffleWindowSize =
getFlags().getReorderFunctionsWindowSize() > 0
On 2015/06/24 00:24:00, John wrote:
> const uint32_t

Done.

[John, 2015-06-24 08:15:55]

https://codereview.chromium.org/1206723003/diff/1/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1206723003/diff/1/src/IceELFObjectWriter.cpp#...
src/IceELFObjectWriter.cpp:514: RandomShuffle(Pool.begin(), Pool.end(),
[this](uint64_t N) {
On 2015/06/24 01:20:40, qining wrote:
> On 2015/06/24 00:24:00, John wrote:
> > Can you capture [Ctx] instead of [this]?
> 
> Done. But it seems I can't capture Ctx or its reference directly as it is a
> member of this class. My workaround is declaring a pointer to Ctx and capture
> the pointer instead. But actually I'm not sure this is better than capturing
> this.

Oh, I was genuinely asking whether capturing [Ctx] would work. I wasn't sure. My
hint that this should not work in C++11 is that capturing [Ctx] actually mean
capturing [this->Ctx], which is not valid.

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:439: const uint32_t ShuffleWindowSize =
getFlags().getReorderFunctionsWindowSize() > 0
line length > 80 chars.

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:455: uint32_t ItemSeq = RawItem->getSequenceNumber();
Optional: I would define ItemSet in line 450, and then I would restructure this
to something like:

if (RawItem == nullptr) {
  RawItem = emitQueueBlockingPop();
  const uint32_t ItemSeq = RawItem->getSequenceNumber();
  if (RawItem == nullptr) {
    ...
  } else if (Threaded && ItemSeq != DesiredSequenceNumber) {
    ...
  } else {
    ...
  }
}

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:463: // ItemSeq == DesiredSequenceNumber, we need to
check if we should
Not necessarily. This else branch will be executed if

!Threaded || ItemSeq == DesiredSequenceNumber.

Does it matter if ItemSeq != DesiredSequenceNumber?

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:481: // keep the blocking profiling workflow unchanged.
There's nothing in the block profiling requiring this to be true -- i.e., you
can emit globals at any time, and profiling should still work. You would need to
change

lowerGlobalsIfNoCodeHasBeenSeen()

to always lower globals, at which point the method would be more suitably named

lowerCurrentAccumulatedGlobals()

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:482: if (Shuffle && !EmitQueueEmpty &&
Personally, I think this would be easier to read:

if (Shuffle) {
  if (!EmitQueueEmpty &&
      !shuffleWindowFull(ShuffleStartIndex, ShuffleEndIndex) &&
      !globalInitsWorkItem(RawItem)) {
    continue;
  }

  RandomShuffle(...);
}

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:545: } // switch (Item->getKind())
Just an observation, I don't see a whole lot of support for commenting what each
} closes throughout the code.

[qining, 2015-06-24 17:17:20]

Fixed some comments in patch set 3.

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:439: const uint32_t ShuffleWindowSize =
getFlags().getReorderFunctionsWindowSize() > 0
On 2015/06/24 08:15:54, John wrote:
> line length > 80 chars.

Done.

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:455: uint32_t ItemSeq = RawItem->getSequenceNumber();
On 2015/06/24 08:15:55, John wrote:
> Optional: I would define ItemSet in line 450, and then I would restructure
this
> to something like:
> 
> if (RawItem == nullptr) {
>   RawItem = emitQueueBlockingPop();
>   const uint32_t ItemSeq = RawItem->getSequenceNumber();
>   if (RawItem == nullptr) {
>     ...
>   } else if (Threaded && ItemSeq != DesiredSequenceNumber) {
>     ...
>   } else {
>     ...
>   }
> }

But there is a problem, RawItem may be a null pointer, perhaps we can use
something like:
const uint32_t ItemSeq = RawItem ? RawItem->getSequenceNumber : 0;

But we need to remember getSequenceNumber() never return 0, which is the case
for now, but will need to be kept for future too.

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:463: // ItemSeq == DesiredSequenceNumber, we need to
check if we should
On 2015/06/24 08:15:54, John wrote:
> Not necessarily. This else branch will be executed if
> 
> !Threaded || ItemSeq == DesiredSequenceNumber.
> 
> Does it matter if ItemSeq != DesiredSequenceNumber?

Yes, you are right, this enclosing else is not necessary.

Actually when !Threaded, DesiredSequenceNumber is usually not equal to ItemSeq.
But we do need to use DesiredSequenceNumber as the index. When Threaded is true,
DesiredSequnceNumber should always equal to ItemSeq in this else branch.

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:481: // keep the blocking profiling workflow unchanged.
On 2015/06/24 08:15:55, John wrote:
> There's nothing in the block profiling requiring this to be true -- i.e., you
> can emit globals at any time, and profiling should still work. You would need
to
> change
> 
> lowerGlobalsIfNoCodeHasBeenSeen()
> 
> to always lower globals, at which point the method would be more suitably
named
> 
> lowerCurrentAccumulatedGlobals()

Yes, you are right, the problem is actually the
lowerGlobalsIfNoCodeHasBeenSeen(). For now, if I don't emit globalinits at the
first, they will (usually) be emitted after all the function code and get a
suffix: "last" in the names of their sections. I've tested the generated code
and it works fine in both xtest and spec2k. The only difference is the name of
the sections and the layout in code. 

However, I personally prefer to keep the lowerGlobalsIfNoCodeHasBeenSeen()
unchanged and always emit global inits first. I think it is safer to change as
fewer things as possible in the current workflow. I hope this reordering doesn't
change anything except applying a buffer just before emitting the functions.

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:482: if (Shuffle && !EmitQueueEmpty &&
On 2015/06/24 08:15:55, John wrote:
> Personally, I think this would be easier to read:
> 
> if (Shuffle) {
>   if (!EmitQueueEmpty &&
>       !shuffleWindowFull(ShuffleStartIndex, ShuffleEndIndex) &&
>       !globalInitsWorkItem(RawItem)) {
>     continue;
>   }
> 
>   RandomShuffle(...);
> }

Done.

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:545: } // switch (Item->getKind())
On 2015/06/24 08:15:54, John wrote:
> Just an observation, I don't see a whole lot of support for commenting what
each
> } closes throughout the code.

Should I remove these labels?

[John, 2015-06-24 17:31:49]

lgtm

wait until others lgtm this too.

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:455: uint32_t ItemSeq = RawItem->getSequenceNumber();
On 2015/06/24 17:17:20, qining wrote:
> On 2015/06/24 08:15:55, John wrote:
> > Optional: I would define ItemSet in line 450, and then I would restructure
> this
> > to something like:
> > 
> > if (RawItem == nullptr) {
> >   RawItem = emitQueueBlockingPop();
> >   const uint32_t ItemSeq = RawItem->getSequenceNumber();
> >   if (RawItem == nullptr) {
> >     ...
> >   } else if (Threaded && ItemSeq != DesiredSequenceNumber) {
> >     ...
> >   } else {
> >     ...
> >   }
> > }
> 
> But there is a problem, RawItem may be a null pointer, perhaps we can use
> something like:
> const uint32_t ItemSeq = RawItem ? RawItem->getSequenceNumber : 0;
> 
> But we need to remember getSequenceNumber() never return 0, which is the case
> for now, but will need to be kept for future too.

Oh, the wonders of early morning coding... =/ nevermind.

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:463: // ItemSeq == DesiredSequenceNumber, we need to
check if we should
On 2015/06/24 17:17:20, qining wrote:
> On 2015/06/24 08:15:54, John wrote:
> > Not necessarily. This else branch will be executed if
> > 
> > !Threaded || ItemSeq == DesiredSequenceNumber.
> > 
> > Does it matter if ItemSeq != DesiredSequenceNumber?
> 
> Yes, you are right, this enclosing else is not necessary.
> 
> Actually when !Threaded, DesiredSequenceNumber is usually not equal to
ItemSeq.
> But we do need to use DesiredSequenceNumber as the index. When Threaded is
true,
> DesiredSequnceNumber should always equal to ItemSeq in this else branch.

I was pointing out that the comment is not accurate, and it may throw someone
off. Perhaps add a sentence explaining that, if !Threaded we're OK with ItemSeq
!= DesiredSequenceNumber.

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:481: // keep the blocking profiling workflow unchanged.
On 2015/06/24 17:17:20, qining wrote:
> On 2015/06/24 08:15:55, John wrote:
> > There's nothing in the block profiling requiring this to be true -- i.e.,
you
> > can emit globals at any time, and profiling should still work. You would
need
> to
> > change
> > 
> > lowerGlobalsIfNoCodeHasBeenSeen()
> > 
> > to always lower globals, at which point the method would be more suitably
> named
> > 
> > lowerCurrentAccumulatedGlobals()
> 
> Yes, you are right, the problem is actually the
> lowerGlobalsIfNoCodeHasBeenSeen(). For now, if I don't emit globalinits at the
> first, they will (usually) be emitted after all the function code and get a
> suffix: "last" in the names of their sections. I've tested the generated code
> and it works fine in both xtest and spec2k. The only difference is the name of
> the sections and the layout in code. 
> 
> However, I personally prefer to keep the lowerGlobalsIfNoCodeHasBeenSeen()
> unchanged and always emit global inits first. I think it is safer to change as
> fewer things as possible in the current workflow. I hope this reordering
doesn't
> change anything except applying a buffer just before emitting the functions. 

Not just that, we don't want to keep an arbitrarily long list of globals around
longer than needed. I was not suggesting you should change that, but that the
comment seems to imply (to me, at least) that there's some invariant requiring
globals to be emitted before everything, or else profiling wouldn't work. Boy, I
really need to work on my communication -- Sorry... :-/

A more accurate comment would read

"... Emit WI_GlobalInits kind block first to avoid holding an arbitrarily large
GlobalDeclarationList."

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:545: } // switch (Item->getKind())
On 2015/06/24 17:17:20, qining wrote:
> On 2015/06/24 08:15:54, John wrote:
> > Just an observation, I don't see a whole lot of support for commenting what
> each
> > } closes throughout the code.
> 
> Should I remove these labels?

I am totally fine with them, I was mostly pointing out to other Subzero authors
as they may have a strong opinion on them.

[jvoung (off chromium), 2015-06-24 17:36:37]

https://codereview.chromium.org/1206723003/diff/20001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1206723003/diff/20001/src/IceClFlags.h#newcod...
src/IceClFlags.h:254: bool ReorderFunctions;
We had previously grouped all of the "bool" options together, and all of the
int-like options together (e.g., RandomNopInsertion vs
RandomNopProbabilityAsPercentage)... I think for alignment / padding efficiency.
At least with types like uint64_t which have wider alignment, we wouldn't want
to interleave bool and uint64_t (not the case here, but an example of why)
because it introduces padding.

Yes there is some akwardness in that the ReorderFunctions will be separate from
ReorderFunctionsWindowSize though...

https://codereview.chromium.org/1206723003/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/reorder-global-variables.ll (right):

https://codereview.chromium.org/1206723003/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-global-variables.ll:3: ; REQUEST: allow_dump
I think this should be "REQUIRES" as the keyword instead of "REQUEST"

https://codereview.chromium.org/1206723003/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-global-variables.ll:15: ; RUN:     -i %s --args
-sz-seed=1 -O2 -reorder-global-variables --skip-unimplemented \
wrap to 80 cols a little earlier if we are wrapping with the "\" =)

[qining, 2015-06-24 20:13:25]

Patchset #3 (id:40001) has been deleted

[qining, 2015-06-24 20:18:56]

Updated with patch set 3.

https://codereview.chromium.org/1206723003/diff/20001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1206723003/diff/20001/src/IceClFlags.h#newcod...
src/IceClFlags.h:254: bool ReorderFunctions;
On 2015/06/24 17:36:37, jvoung wrote:
> We had previously grouped all of the "bool" options together, and all of the
> int-like options together (e.g., RandomNopInsertion vs
> RandomNopProbabilityAsPercentage)... I think for alignment / padding
efficiency.
> At least with types like uint64_t which have wider alignment, we wouldn't want
> to interleave bool and uint64_t (not the case here, but an example of why)
> because it introduces padding.
> 
> Yes there is some akwardness in that the ReorderFunctions will be separate
from
> ReorderFunctionsWindowSize though...

Done.

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:463: // ItemSeq == DesiredSequenceNumber, we need to
check if we should
On 2015/06/24 17:31:48, John wrote:
> On 2015/06/24 17:17:20, qining wrote:
> > On 2015/06/24 08:15:54, John wrote:
> > > Not necessarily. This else branch will be executed if
> > > 
> > > !Threaded || ItemSeq == DesiredSequenceNumber.
> > > 
> > > Does it matter if ItemSeq != DesiredSequenceNumber?
> > 
> > Yes, you are right, this enclosing else is not necessary.
> > 
> > Actually when !Threaded, DesiredSequenceNumber is usually not equal to
> ItemSeq.
> > But we do need to use DesiredSequenceNumber as the index. When Threaded is
> true,
> > DesiredSequnceNumber should always equal to ItemSeq in this else branch.
> 
> I was pointing out that the comment is not accurate, and it may throw someone
> off. Perhaps add a sentence explaining that, if !Threaded we're OK with
ItemSeq
> != DesiredSequenceNumber.

Done. Yes, the original comment is not accurate, I should mention the !Threaded
case.

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:481: // keep the blocking profiling workflow unchanged.
On 2015/06/24 17:31:49, John wrote:
> On 2015/06/24 17:17:20, qining wrote:
> > On 2015/06/24 08:15:55, John wrote:
> > > There's nothing in the block profiling requiring this to be true -- i.e.,
> you
> > > can emit globals at any time, and profiling should still work. You would
> need
> > to
> > > change
> > > 
> > > lowerGlobalsIfNoCodeHasBeenSeen()
> > > 
> > > to always lower globals, at which point the method would be more suitably
> > named
> > > 
> > > lowerCurrentAccumulatedGlobals()
> > 
> > Yes, you are right, the problem is actually the
> > lowerGlobalsIfNoCodeHasBeenSeen(). For now, if I don't emit globalinits at
the
> > first, they will (usually) be emitted after all the function code and get a
> > suffix: "last" in the names of their sections. I've tested the generated
code
> > and it works fine in both xtest and spec2k. The only difference is the name
of
> > the sections and the layout in code. 
> > 
> > However, I personally prefer to keep the lowerGlobalsIfNoCodeHasBeenSeen()
> > unchanged and always emit global inits first. I think it is safer to change
as
> > fewer things as possible in the current workflow. I hope this reordering
> doesn't
> > change anything except applying a buffer just before emitting the functions.

> 
> Not just that, we don't want to keep an arbitrarily long list of globals
around
> longer than needed. I was not suggesting you should change that, but that the
> comment seems to imply (to me, at least) that there's some invariant requiring
> globals to be emitted before everything, or else profiling wouldn't work. Boy,
I
> really need to work on my communication -- Sorry... :-/
> 
> A more accurate comment would read
> 
> "... Emit WI_GlobalInits kind block first to avoid holding an arbitrarily
large
> GlobalDeclarationList."

Done. I do think this is more of a communication problem on my side...

https://codereview.chromium.org/1206723003/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:545: } // switch (Item->getKind())
On 2015/06/24 17:31:49, John wrote:
> On 2015/06/24 17:17:20, qining wrote:
> > On 2015/06/24 08:15:54, John wrote:
> > > Just an observation, I don't see a whole lot of support for commenting
what
> > each
> > > } closes throughout the code.
> > 
> > Should I remove these labels?
> 
> I am totally fine with them, I was mostly pointing out to other Subzero
authors
> as they may have a strong opinion on them.

Done. Just removed them, they look more likely my personal draft.

https://codereview.chromium.org/1206723003/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/reorder-global-variables.ll (right):

https://codereview.chromium.org/1206723003/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-global-variables.ll:3: ; REQUEST: allow_dump
On 2015/06/24 17:36:37, jvoung wrote:
> I think this should be "REQUIRES" as the keyword instead of "REQUEST"

Done.

https://codereview.chromium.org/1206723003/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-global-variables.ll:15: ; RUN:     -i %s --args
-sz-seed=1 -O2 -reorder-global-variables --skip-unimplemented \
On 2015/06/24 17:36:37, jvoung wrote:
> wrap to 80 cols a little earlier if we are wrapping with the "\" =)

Done.

[jvoung (off chromium), 2015-06-24 21:26:24]

otherwise LGTM

https://codereview.chromium.org/1206723003/diff/60001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1206723003/diff/60001/src/IceClFlags.h#newcod...
src/IceClFlags.h:239: size_t NumTranslationThreads; // 0 means completely
sequential
Let's leave size_t and the uint64_t where it was before. size_t is sometimes
64-bit.

https://codereview.chromium.org/1206723003/diff/60001/src/IceClFlags.h#newcod...
src/IceClFlags.h:244: uint64_t RandomSeed;
Same -- Otherwise this is interleaving 32-bit things with 64-bit things, which
will leave padding gaps in between, unless you are lucky and end up with an even
number of 32-bit things before the 64-bit thing.

[qining, 2015-06-24 23:22:34]

Move back the NumTranslationThreads and RandomSeed to their original location.

https://codereview.chromium.org/1206723003/diff/60001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1206723003/diff/60001/src/IceClFlags.h#newcod...
src/IceClFlags.h:239: size_t NumTranslationThreads; // 0 means completely
sequential
On 2015/06/24 21:26:23, jvoung wrote:
> Let's leave size_t and the uint64_t where it was before. size_t is sometimes
> 64-bit.
> 

Done.

https://codereview.chromium.org/1206723003/diff/60001/src/IceClFlags.h#newcod...
src/IceClFlags.h:244: uint64_t RandomSeed;
On 2015/06/24 21:26:23, jvoung wrote:
> Same -- Otherwise this is interleaving 32-bit things with 64-bit things, which
> will leave padding gaps in between, unless you are lucky and end up with an
even
> number of 32-bit things before the 64-bit thing.

Done.

[Jim Stichnoth, 2015-06-25 13:20:59]

https://codereview.chromium.org/1206723003/diff/60001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1206723003/diff/60001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:296: // Command line option for turing on global variable
layout reordering.
turning

https://codereview.chromium.org/1206723003/diff/60001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:307: } // end of anonymous namespace
Can we keep the blank line above the closing brace?

https://codereview.chromium.org/1206723003/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1206723003/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:439: const uint32_t ShuffleWindowSize =
How about:
  std::max(1, getFlags().getReorderFunctionsWindowSize())

https://codereview.chromium.org/1206723003/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:443: bool Shuffle = Threaded ?
getFlags().shouldReorderFunctions() : false;
How about:
  Threaded && getFlags().shouldReorderFunctions()

https://codereview.chromium.org/1206723003/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:470: // We have the desired EmitterWorkItem or an
nullptr as the end notifier.
"a nullptr", or just "nullptr"

https://codereview.chromium.org/1206723003/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:531: if (!ALLOW_DUMP)
Change ALLOW_DUMP to BuildDefs::dump() after rebasing.

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/reorder-functions.ll (right):

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-functions.ll:1: ; This is a smoke test of
function reordering.
Some fairly shallow comments:

1. I think it would be easier for me to read and recognize the differences
between tests if, for each of the 4 separate tests, you put the Om1 and O2 cases
together, and a blank line between each Om1/O2 set.  E.g.:

RUN: %p2i ... -O2 ... --check-prefix=DEFAULTWINDOWSIZE
RUN: %p2i ... -Om1 ... --check-prefix=DEFAULTWINDOWSIZE

RUN: %p2i ... -O2 ... --check-prefix=WINDOWSIZE1
RUN: %p2i ... -Om1 ... --check-prefix=WINDOWSIZE1

RUN: %p2i ... -O2 ... --check-prefix=SEQUENTIAL
RUN: %p2i ... -Om1 ... --check-prefix=SEQUENTIAL

RUN: %p2i ... -O2 ... --check-prefix=WINDOWSIZEMAX
RUN: %p2i ... -Om1 ... --check-prefix=WINDOWSIZEMAX

2. I would rename the functions something like func1, func2, func3.  Easier to
"see" the permutation in the CHECK lines.

3. Maybe simplify the 6 functions?  They could all just be like:

define void func1() {
  ret void
}

That would make it easier to focus on the aspect being tested.

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-functions.ll:110: 
Git often complains about "trailing whitespace" when there are blank lines at
the end of the file.

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/reorder-global-variables.ll (right):

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-global-variables.ll:56: define internal i32
@main(i32 %argc, i32 %argv) {
I think this test could be simplified by removing all the functions, leaving
just the global declarations/definitions.

Also, you could probably remove the @__* globals, as they are basically just
left over from some automatic C-to-bitcode conversion script.

If you remove @__*, you can probably combine the ASM and OBJ filecheck prefixes
into a single set of check lines.

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-global-variables.ll:162: 
trailing blank line

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/reorder-pooled-constants.ll (right):

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-pooled-constants.ll:545: ; X86O2:     55555555
5555d53f 00000000 0000f8ff
I would not indent this - be consistent with the .cst4 lines.

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-pooled-constants.ll:553: ; X86OM1: 0000803f
0000003f 00000040 0000c0ff
I am thoroughly puzzled as to why the Om1 ordering would be different from the
O2 ordering, given the same seed.  Any ideas?

[qining, 2015-06-25 21:10:11]

Updated in patch set 5.

https://codereview.chromium.org/1206723003/diff/60001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1206723003/diff/60001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:296: // Command line option for turing on global variable
layout reordering.
On 2015/06/25 13:20:59, stichnot wrote:
> turning

Done.

https://codereview.chromium.org/1206723003/diff/60001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:307: } // end of anonymous namespace
On 2015/06/25 13:20:59, stichnot wrote:
> Can we keep the blank line above the closing brace?

Done.

https://codereview.chromium.org/1206723003/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1206723003/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:439: const uint32_t ShuffleWindowSize =
On 2015/06/25 13:20:59, stichnot wrote:
> How about:
>   std::max(1, getFlags().getReorderFunctionsWindowSize())

Done. Before rebasing, I need to include <algorithm>, but I don't need to do so
after rebasing, and I can't find any header includes <algorithm>. How do we
include this header?

https://codereview.chromium.org/1206723003/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:443: bool Shuffle = Threaded ?
getFlags().shouldReorderFunctions() : false;
On 2015/06/25 13:20:59, stichnot wrote:
> How about:
>   Threaded && getFlags().shouldReorderFunctions()

Done.

https://codereview.chromium.org/1206723003/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:470: // We have the desired EmitterWorkItem or an
nullptr as the end notifier.
On 2015/06/25 13:20:59, stichnot wrote:
> "a nullptr", or just "nullptr"

Done.

https://codereview.chromium.org/1206723003/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:531: if (!ALLOW_DUMP)
On 2015/06/25 13:20:59, stichnot wrote:
> Change ALLOW_DUMP to BuildDefs::dump() after rebasing.

Done.

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/reorder-functions.ll (right):

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-functions.ll:1: ; This is a smoke test of
function reordering.
On 2015/06/25 13:20:59, stichnot wrote:
> Some fairly shallow comments:
> 
> 1. I think it would be easier for me to read and recognize the differences
> between tests if, for each of the 4 separate tests, you put the Om1 and O2
cases
> together, and a blank line between each Om1/O2 set.  E.g.:
> 
> RUN: %p2i ... -O2 ... --check-prefix=DEFAULTWINDOWSIZE
> RUN: %p2i ... -Om1 ... --check-prefix=DEFAULTWINDOWSIZE
> 
> RUN: %p2i ... -O2 ... --check-prefix=WINDOWSIZE1
> RUN: %p2i ... -Om1 ... --check-prefix=WINDOWSIZE1
> 
> RUN: %p2i ... -O2 ... --check-prefix=SEQUENTIAL
> RUN: %p2i ... -Om1 ... --check-prefix=SEQUENTIAL
> 
> RUN: %p2i ... -O2 ... --check-prefix=WINDOWSIZEMAX
> RUN: %p2i ... -Om1 ... --check-prefix=WINDOWSIZEMAX
> 
> 2. I would rename the functions something like func1, func2, func3.  Easier to
> "see" the permutation in the CHECK lines.
> 
> 3. Maybe simplify the 6 functions?  They could all just be like:
> 
> define void func1() {
>   ret void
> }
> 
> That would make it easier to focus on the aspect being tested.

Done.

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-functions.ll:110: 
On 2015/06/25 13:20:59, stichnot wrote:
> Git often complains about "trailing whitespace" when there are blank lines at
> the end of the file.

Done.

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/reorder-global-variables.ll (right):

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-global-variables.ll:56: define internal i32
@main(i32 %argc, i32 %argv) {
On 2015/06/25 13:20:59, stichnot wrote:
> I think this test could be simplified by removing all the functions, leaving
> just the global declarations/definitions.
> 
> Also, you could probably remove the @__* globals, as they are basically just
> left over from some automatic C-to-bitcode conversion script.
> 
> If you remove @__*, you can probably combine the ASM and OBJ filecheck
prefixes
> into a single set of check lines.

Done.

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-global-variables.ll:162: 
On 2015/06/25 13:20:59, stichnot wrote:
> trailing blank line

Done.

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/reorder-pooled-constants.ll (right):

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-pooled-constants.ll:545: ; X86O2:     55555555
5555d53f 00000000 0000f8ff
On 2015/06/25 13:20:59, stichnot wrote:
> I would not indent this - be consistent with the .cst4 lines.

Done.

[Jim Stichnoth, 2015-06-26 01:54:57]

https://codereview.chromium.org/1206723003/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1206723003/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:439: const uint32_t ShuffleWindowSize =
On 2015/06/25 21:10:10, qining wrote:
> On 2015/06/25 13:20:59, stichnot wrote:
> > How about:
> >   std::max(1, getFlags().getReorderFunctionsWindowSize())
> 
> Done. Before rebasing, I need to include <algorithm>, but I don't need to do
so
> after rebasing, and I can't find any header includes <algorithm>. How do we
> include this header?

Some other header (LLVM?) must be including it.  But I would go ahead and
include it in this file as well.

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/reorder-functions.ll (right):

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-functions.ll:1: ; This is a smoke test of
function reordering.
On 2015/06/25 21:10:10, qining wrote:
> On 2015/06/25 13:20:59, stichnot wrote:
> > Some fairly shallow comments:
> > 
> > 1. I think it would be easier for me to read and recognize the differences
> > between tests if, for each of the 4 separate tests, you put the Om1 and O2
> cases
> > together, and a blank line between each Om1/O2 set.  E.g.:
> > 
> > RUN: %p2i ... -O2 ... --check-prefix=DEFAULTWINDOWSIZE
> > RUN: %p2i ... -Om1 ... --check-prefix=DEFAULTWINDOWSIZE
> > 
> > RUN: %p2i ... -O2 ... --check-prefix=WINDOWSIZE1
> > RUN: %p2i ... -Om1 ... --check-prefix=WINDOWSIZE1
> > 
> > RUN: %p2i ... -O2 ... --check-prefix=SEQUENTIAL
> > RUN: %p2i ... -Om1 ... --check-prefix=SEQUENTIAL
> > 
> > RUN: %p2i ... -O2 ... --check-prefix=WINDOWSIZEMAX
> > RUN: %p2i ... -Om1 ... --check-prefix=WINDOWSIZEMAX
> > 
> > 2. I would rename the functions something like func1, func2, func3.  Easier
to
> > "see" the permutation in the CHECK lines.
> > 
> > 3. Maybe simplify the 6 functions?  They could all just be like:
> > 
> > define void func1() {
> >   ret void
> > }
> > 
> > That would make it easier to focus on the aspect being tested.
> 
> Done.

You only addressed #1, while #2 and #3 are the same as before...

https://codereview.chromium.org/1206723003/diff/100001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1206723003/diff/100001/src/IceGlobalContext.c...
src/IceGlobalContext.cpp:440: std::max((uint32_t)1,
getFlags().getReorderFunctionsWindowSize());
I think you can just use 1u instead of casting.

https://codereview.chromium.org/1206723003/diff/100001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/reorder-pooled-constants.ll (right):

https://codereview.chromium.org/1206723003/diff/100001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/reorder-pooled-constants.ll:536: ; TODO(qining,
stichnot): Om1 and O2 result into different sizes of i32 constant
This isn't really a TODO, just an explanation of the Om1/O2 differences.

[qining, 2015-06-26 03:16:08]

Updated with patch set 6.

https://codereview.chromium.org/1206723003/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1206723003/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:439: const uint32_t ShuffleWindowSize =
On 2015/06/26 01:54:57, stichnot wrote:
> On 2015/06/25 21:10:10, qining wrote:
> > On 2015/06/25 13:20:59, stichnot wrote:
> > > How about:
> > >   std::max(1, getFlags().getReorderFunctionsWindowSize())
> > 
> > Done. Before rebasing, I need to include <algorithm>, but I don't need to do
> so
> > after rebasing, and I can't find any header includes <algorithm>. How do we
> > include this header?
> 
> Some other header (LLVM?) must be including it.  But I would go ahead and
> include it in this file as well.

Done.

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/reorder-functions.ll (right):

https://codereview.chromium.org/1206723003/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-functions.ll:1: ; This is a smoke test of
function reordering.
On 2015/06/26 01:54:57, stichnot wrote:
> On 2015/06/25 21:10:10, qining wrote:
> > On 2015/06/25 13:20:59, stichnot wrote:
> > > Some fairly shallow comments:
> > > 
> > > 1. I think it would be easier for me to read and recognize the differences
> > > between tests if, for each of the 4 separate tests, you put the Om1 and O2
> > cases
> > > together, and a blank line between each Om1/O2 set.  E.g.:
> > > 
> > > RUN: %p2i ... -O2 ... --check-prefix=DEFAULTWINDOWSIZE
> > > RUN: %p2i ... -Om1 ... --check-prefix=DEFAULTWINDOWSIZE
> > > 
> > > RUN: %p2i ... -O2 ... --check-prefix=WINDOWSIZE1
> > > RUN: %p2i ... -Om1 ... --check-prefix=WINDOWSIZE1
> > > 
> > > RUN: %p2i ... -O2 ... --check-prefix=SEQUENTIAL
> > > RUN: %p2i ... -Om1 ... --check-prefix=SEQUENTIAL
> > > 
> > > RUN: %p2i ... -O2 ... --check-prefix=WINDOWSIZEMAX
> > > RUN: %p2i ... -Om1 ... --check-prefix=WINDOWSIZEMAX
> > > 
> > > 2. I would rename the functions something like func1, func2, func3. 
Easier
> to
> > > "see" the permutation in the CHECK lines.
> > > 
> > > 3. Maybe simplify the 6 functions?  They could all just be like:
> > > 
> > > define void func1() {
> > >   ret void
> > > }
> > > 
> > > That would make it easier to focus on the aspect being tested.
> > 
> > Done.
> 
> You only addressed #1, while #2 and #3 are the same as before...

Done. Sorry, I should be more careful.

https://codereview.chromium.org/1206723003/diff/100001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1206723003/diff/100001/src/IceGlobalContext.c...
src/IceGlobalContext.cpp:440: std::max((uint32_t)1,
getFlags().getReorderFunctionsWindowSize());
On 2015/06/26 01:54:57, stichnot wrote:
> I think you can just use 1u instead of casting.

Done. This is much better.

https://codereview.chromium.org/1206723003/diff/100001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/reorder-pooled-constants.ll (right):

https://codereview.chromium.org/1206723003/diff/100001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/reorder-pooled-constants.ll:536: ; TODO(qining,
stichnot): Om1 and O2 result into different sizes of i32 constant
On 2015/06/26 01:54:57, stichnot wrote:
> This isn't really a TODO, just an explanation of the Om1/O2 differences.

Done. Add following sentence to the top:
TODO(qining, stichnot): Unify the checkers for Om1 and O2 optimization levels.

[Jim Stichnoth, 2015-06-26 06:14:42]

otherwise lgtm

https://codereview.chromium.org/1206723003/diff/120001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1206723003/diff/120001/src/IceGlobalContext.c...
src/IceGlobalContext.cpp:32: #include <algorithm>
Sort this alphabetically, i.e. above ctype.h.

And while you're doing that, can you also change <ctype.h> to <cctype>?

[Jim Stichnoth, 2015-06-26 06:17:37]

Forgot to mention - the title of the CL (both the Subject field and the first
line of the Description field) should be less than 80 characters, per git
guidelines.

[qining, 2015-06-26 16:19:44]

Changed the title and first line in description to 79-character long.

Updated in patch set 7

[qining, 2015-06-26 16:20:14]

https://codereview.chromium.org/1206723003/diff/120001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1206723003/diff/120001/src/IceGlobalContext.c...
src/IceGlobalContext.cpp:32: #include <algorithm>
On 2015/06/26 06:14:42, stichnot wrote:
> Sort this alphabetically, i.e. above ctype.h.
> 
> And while you're doing that, can you also change <ctype.h> to <cctype>?

Done.

[qining, 2015-06-26 16:36:07]

Committed patchset #7 (id:140001) manually as
7cd5351c07806a1bc7ba1c1aec3380f7b715bda8 (presubmit successful).