Add UBSAN build option and fix undefined behaviour errors.

Add UBSAN build option and fix undefined behaviour errors.

1. The assembler tried to write to unaligned addresses but memcpy fixes this.
2. The InstKind and OperandKind enums allowed target specific kinds that did not
   fall in the defined range. Defining the maximum target kind in the enum sorts
   this problem.

BUG=
R=stichnot@chromium.org

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

[ascull, 2015-09-04 20:55:46]

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

[Jim Stichnoth, 2015-09-04 21:32:49]

https://codereview.chromium.org/1311653003/diff/20001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1311653003/diff/20001/src/IceAssembler.h#newc...
src/IceAssembler.h:103: memcpy(reinterpret_cast<void *>(Cursor), &Value,
sizeof(T));
Have you checked the effect of this patch on assembler performance?  Most of the
calls to emit() originate from emitUint8() where sizeof(T)==1.

I would assume that since sizeof(T) is always known, the compiler will be smart
enough not to actually call memcpy, especially for sizeof(T)==1, but no
guarantee.

If there is a performance difference, you could probably do
  if (sizeof(T) == 1)  /* direct copy */ ;
  else /* memcpy() */ ;
which would be less duplication than specializing the template.

Also, for future developers, leave a comment that memcpy() is to avoid
misalignment.

https://codereview.chromium.org/1311653003/diff/20001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1311653003/diff/20001/src/IceInst.h#newcode70
src/IceInst.h:70: // target-specific low-level ICE Anything >= Target is an
InstTarget
probably delete the "target-specific low-level ICE"

https://codereview.chromium.org/1311653003/diff/20001/src/IceInst.h#newcode75
src/IceInst.h:75: TARGETKINDS_INSTS_TABLE
IIRC, you are solving the problem by defining Target0, Target1, ..., Target100,
but you only actually use Target0, leaving the rest just as placeholders.

Could you instead do something like:
  JumpTable,
  Target,
  Target_Max = Target + 100
and then after the target instruction enum definition, static_assert that the
target enum max value is no greater than Target_Max?

https://codereview.chromium.org/1311653003/diff/20001/src/IceTargetKinds.def
File src/IceTargetKinds.def (right):

https://codereview.chromium.org/1311653003/diff/20001/src/IceTargetKinds.def#...
src/IceTargetKinds.def:1: //===- subzero/src/IceTargetKinds.def - X-macros of
target kinds -*- C++ -*-===//
80-col

[ascull, 2015-09-05 00:00:48]

https://codereview.chromium.org/1311653003/diff/20001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1311653003/diff/20001/src/IceAssembler.h#newc...
src/IceAssembler.h:103: memcpy(reinterpret_cast<void *>(Cursor), &Value,
sizeof(T));
On 2015/09/04 21:32:48, stichnot wrote:
> Have you checked the effect of this patch on assembler performance?  Most of
the
> calls to emit() originate from emitUint8() where sizeof(T)==1.
> 
> I would assume that since sizeof(T) is always known, the compiler will be
smart
> enough not to actually call memcpy, especially for sizeof(T)==1, but no
> guarantee.
> 
> If there is a performance difference, you could probably do
>   if (sizeof(T) == 1)  /* direct copy */ ;
>   else /* memcpy() */ ;
> which would be less duplication than specializing the template.
> 
> Also, for future developers, leave a comment that memcpy() is to avoid
> misalignment.

The binary produces is the same before and after this change. (For some reason
the binary changes after the enum stuff which I didn't expect)

The comment has been added.

https://codereview.chromium.org/1311653003/diff/20001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1311653003/diff/20001/src/IceInst.h#newcode70
src/IceInst.h:70: // target-specific low-level ICE Anything >= Target is an
InstTarget
On 2015/09/04 21:32:48, stichnot wrote:
> probably delete the "target-specific low-level ICE"

Done.

https://codereview.chromium.org/1311653003/diff/20001/src/IceInst.h#newcode75
src/IceInst.h:75: TARGETKINDS_INSTS_TABLE
On 2015/09/04 21:32:49, stichnot wrote:
> IIRC, you are solving the problem by defining Target0, Target1, ...,
Target100,
> but you only actually use Target0, leaving the rest just as placeholders.
> 
> Could you instead do something like:
>   JumpTable,
>   Target,
>   Target_Max = Target + 100
> and then after the target instruction enum definition, static_assert that the
> target enum max value is no greater than Target_Max?

I'm not sure I see what you mean here. You need to explicitly define the
placeholders to avoid UB. Or are you just suggesting to add Target_Max as a
value?

https://codereview.chromium.org/1311653003/diff/20001/src/IceTargetKinds.def
File src/IceTargetKinds.def (right):

https://codereview.chromium.org/1311653003/diff/20001/src/IceTargetKinds.def#...
src/IceTargetKinds.def:1: //===- subzero/src/IceTargetKinds.def - X-macros of
target kinds -*- C++ -*-===//
On 2015/09/04 21:32:49, stichnot wrote:
> 80-col

Done.

[Jim Stichnoth, 2015-09-05 01:03:22]

https://codereview.chromium.org/1311653003/diff/20001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1311653003/diff/20001/src/IceAssembler.h#newc...
src/IceAssembler.h:103: memcpy(reinterpret_cast<void *>(Cursor), &Value,
sizeof(T));
On 2015/09/05 00:00:48, ascull wrote:
> On 2015/09/04 21:32:48, stichnot wrote:
> > Have you checked the effect of this patch on assembler performance?  Most of
> the
> > calls to emit() originate from emitUint8() where sizeof(T)==1.
> > 
> > I would assume that since sizeof(T) is always known, the compiler will be
> smart
> > enough not to actually call memcpy, especially for sizeof(T)==1, but no
> > guarantee.
> > 
> > If there is a performance difference, you could probably do
> >   if (sizeof(T) == 1)  /* direct copy */ ;
> >   else /* memcpy() */ ;
> > which would be less duplication than specializing the template.
> > 
> > Also, for future developers, leave a comment that memcpy() is to avoid
> > misalignment.
> 
> The binary produces is the same before and after this change. (For some reason
> the binary changes after the enum stuff which I didn't expect)
> 
> The comment has been added.

OK, thanks for checking.

https://codereview.chromium.org/1311653003/diff/20001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1311653003/diff/20001/src/IceInst.h#newcode75
src/IceInst.h:75: TARGETKINDS_INSTS_TABLE
On 2015/09/05 00:00:48, ascull wrote:
> On 2015/09/04 21:32:49, stichnot wrote:
> > IIRC, you are solving the problem by defining Target0, Target1, ...,
> Target100,
> > but you only actually use Target0, leaving the rest just as placeholders.
> > 
> > Could you instead do something like:
> >   JumpTable,
> >   Target,
> >   Target_Max = Target + 100
> > and then after the target instruction enum definition, static_assert that
the
> > target enum max value is no greater than Target_Max?
> 
> I'm not sure I see what you mean here. You need to explicitly define the
> placeholders to avoid UB. Or are you just suggesting to add Target_Max as a
> value?

I believe the UB comes from trying to represent an enum value outside the enum's
min/max range, not about "holes" in the specified enum values.  For example,
consider this program:

#include <cstdio>

enum TestEnum {
  v10 = 10,
  v20 = 20,
  v30 = 30
};

void consume(TestEnum te) {
  printf("%d\n", te);
}

int main(int argc, char **argv) {
  TestEnum te;
  te = (TestEnum)5;
  consume(te);
  te = (TestEnum)10;
  consume(te);
  te = (TestEnum)15;
  consume(te);
  te = (TestEnum)25;
  consume(te);
  te = (TestEnum)30;
  consume(te);
  te = (TestEnum)35;
  consume(te);
  return 0;
}

UBSan complains about the 35, but not about 15 or 25, or, curiously, 5.

Also a stackoverflow question:
http://stackoverflow.com/questions/7676817/forcing-improper-enum-values

While we're at it, I think 100 may be too small for the number of
target-specific instructions.  Maybe just set the max enum value to 255?  (or (1
<< CHAR_BIT) - 1 if we're avoiding magic constants)?

[John, 2015-09-08 15:57:11]

https://codereview.chromium.org/1311653003/diff/40001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1311653003/diff/40001/src/IceInst.h#newcode74
src/IceInst.h:74: #define X(n) Target##n,
If I understand what Jim said, the UB happens not when storing an invalid value,
but when storing an out-of-range value. If that's the case, then I would say
just declare a 

  Target  = ???
  MaxTargetInst = 255,

and then change setKind to

assert(Kind <= MaxTargetInst);

[ascull, 2015-09-08 17:29:26]

https://codereview.chromium.org/1311653003/diff/20001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1311653003/diff/20001/src/IceInst.h#newcode75
src/IceInst.h:75: TARGETKINDS_INSTS_TABLE
On 2015/09/05 01:03:22, stichnot wrote:
> On 2015/09/05 00:00:48, ascull wrote:
> > On 2015/09/04 21:32:49, stichnot wrote:
> > > IIRC, you are solving the problem by defining Target0, Target1, ...,
> > Target100,
> > > but you only actually use Target0, leaving the rest just as placeholders.
> > > 
> > > Could you instead do something like:
> > >   JumpTable,
> > >   Target,
> > >   Target_Max = Target + 100
> > > and then after the target instruction enum definition, static_assert that
> the
> > > target enum max value is no greater than Target_Max?
> > 
> > I'm not sure I see what you mean here. You need to explicitly define the
> > placeholders to avoid UB. Or are you just suggesting to add Target_Max as a
> > value?
> 
> I believe the UB comes from trying to represent an enum value outside the
enum's
> min/max range, not about "holes" in the specified enum values.  For example,
> consider this program:
> 
> #include <cstdio>
> 
> enum TestEnum {
>   v10 = 10,
>   v20 = 20,
>   v30 = 30
> };
> 
> void consume(TestEnum te) {
>   printf("%d\n", te);
> }
> 
> int main(int argc, char **argv) {
>   TestEnum te;
>   te = (TestEnum)5;
>   consume(te);
>   te = (TestEnum)10;
>   consume(te);
>   te = (TestEnum)15;
>   consume(te);
>   te = (TestEnum)25;
>   consume(te);
>   te = (TestEnum)30;
>   consume(te);
>   te = (TestEnum)35;
>   consume(te);
>   return 0;
> }
> 
> UBSan complains about the 35, but not about 15 or 25, or, curiously, 5.
> 
> Also a stackoverflow question:
> http://stackoverflow.com/questions/7676817/forcing-improper-enum-values
> 
> While we're at it, I think 100 may be too small for the number of
> target-specific instructions.  Maybe just set the max enum value to 255?  (or
(1
> << CHAR_BIT) - 1 if we're avoiding magic constants)?

Done.

https://codereview.chromium.org/1311653003/diff/40001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1311653003/diff/40001/src/IceInst.h#newcode74
src/IceInst.h:74: #define X(n) Target##n,
On 2015/09/08 15:57:11, John wrote:
> If I understand what Jim said, the UB happens not when storing an invalid
value,
> but when storing an out-of-range value. If that's the case, then I would say
> just declare a 
> 
>   Target  = ???
>   MaxTargetInst = 255,
> 
> and then change setKind to
> 
> assert(Kind <= MaxTargetInst);
> 

Done.

[Jim Stichnoth, 2015-09-08 22:10:32]

lgtm

https://codereview.chromium.org/1311653003/diff/60001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1311653003/diff/60001/src/IceOperand.h#newcode54
src/IceOperand.h:54: kTarget_Max = kTarget + MaxTargetKinds,
What do you think of setting kTarget_Max = uint8_t::max(), plus a static_assert
that kTarget<=kTarget_Max?

[ascull, 2015-09-09 22:50:46]

Committed patchset #5 (id:80001) manually as
6ef7949448bbe3fdb1a0cf3dcbd32fd46c4baf9d (presubmit successful).