[llvm-commits] [PATCH] TableGen backend support to express relations between instruction

[Jyotsna Verma]

Hello Sean,

Thanks for reviewing the patch.
Problem with this code is 'start, mid and end' being 'unsigned'. Had they
been declared 'int', there wouldn't be any overflow problem on 'end =
mid-1'. I'll change 'mid = (start+end)/2' to 'mid=start+(end-start)/2' for
the valid reasons you've pointed out.

Thanks,
Jyotsna

> +void MapTableEmitter::emitBinSearch(raw_ostream &OS, unsigned
> +TableSize) {
> +  OS << "  unsigned mid;\n";
> +  OS << "  unsigned start = 0;\n";
> +  OS << "  unsigned end = " << TableSize - 1 << ";\n";
> +  OS << "  while (start <= end) {\n";
> +  OS << "    mid = (end + start)/2;\n";
> +  OS << "    if (Opcode == " << InstrMapDesc.getName() << "Table[mid][0])
> {\n";
> +  OS << "      break;\n";
> +  OS << "    }\n";
> +  OS << "    if (Opcode < " << InstrMapDesc.getName() <<
> "Table[mid][0])\n";
> +  OS << "      end = mid - 1;\n";
> +  OS << "    else\n";
> +  OS << "      start = mid + 1;\n";
> +  OS << "  }\n";
> +  OS << "  if (start > end)\n";
> +  OS << "    return -1; // Instruction doesn't exist in this table.\n\n";
> +}
> 
> This code is buggy. In `end = mid - 1` could evaluate to UINT_MAX in the
case
> of `TableSize == 2`, since `start` will be initialized to 0, `end` will be
initialized
> to `1`, so `mid` will be `(0+1)/2` which is 0. Then if the given opcode is
less
> than the value in the table, you will perform `end = mid - 1` which gives
> UINT_MAX.
> 
> I recommend using half-open ranges with a loop invariant "the element we
> are looking for, if present, is in the range [start,end)", and the
termination
> condition is `start == end`. Also, `mid = (end + start)/2` is potentially
> dangerous, since (end+start) could overflow an unsigned and then dividing
> by 2 will give an undesired result; it is better to do `mid = start + (end
-
> start)/2`. Alternatively, if you know that you can count on <algorithm> in
this
> code, just use std::lower_bound().
> 
> Also, I'm not sure if this routine is meant to return `-1` as an error
code (are
> clients going to be checking this?). If clients are not expected to check
this,
> then you should probably assert that an entry was found in the table.