7862 libdisasm: left shift of negative value

Review Request #356 — Created Feb. 14, 2017 and submitted

tsoome
illumos-gate
7862
949f1c8...
general

7862 libdisasm: left shift of negative value

for an reference, https://en.wikipedia.org/wiki/Two%27s_complement



  • 0
  • 0
  • 3
  • 0
  • 3
Description From Last Updated
andy_js
  1. Ship It!
  2. 
      
marcel
  1. 
      
  2. Wouldn't be UINT32_MAX better?
    1. hm, I do not mind, the function does take and return explitic 32bit int, so it only makes sense to use UINT32_MAX here. My concern was if we might get into trouble with something like kmdb, but this should not be the case either.

  3. 
      
tsoome
marcel
  1. Ship It!
  2. 
      
jbk
  1. Ship It!
  2. 
      
daleg
  1. Ship It!
  2. 
      
rm
  1. 
      
  2. Is this broader function sound? If we're shifting an unsigned value and then oring in values, we're going to have an unsigned value, but then we'll end up casting it back to a signed value as part of returning it.
    1. I do not see how it is broader. First of all, the -1L means 0xFFFFFFFF, which is the same as UINT_MAX and UNINT32_MAX. Of course what the compiler is trying to tell us is that bitwise shift to left is not defined for negative values, so -1L << X is illegal, but happens to work. In an sense the replacement does implement the same assumption about how the negative value is represented internally - namely, having high bits set. So the both solutions appear to work, but are making an assumption about how the type is represented in the memory.

      I don't know, maybe we should use some alternate method there and avoid bitshifting at all, but tbh, I do not really see the huge benefit about it.

    2. Alternative take, still using bitwise operations, but no constant. I think this one is much better:)

  3. 
      
tsoome
tsoome
marcel
  1. Ship It!
  2. 
      
rm
  1. Ship It!
  2. 
      
daleg
  1. Ship It!
  2. 
      
vgusev
  1. 
      
  2. If you consider "bits" can be negative, then (1<< bits) has undefined behavior by standard.

    1. As this is static function, called only from this module and all cases are using positive constants, I did change bits to uint32_t.

    2. It looks better. I would recommend to add some kind of ASSERT() to make sure that @bits is not zero.

    3. added ASSERT0(), as the same is used in dis_s390x.c, so we have some consistency.

  3. 
      
tsoome
rm
  1. Ship It!
  2. 
      
tsoome
marcel
  1. Ship It!
  2. usr/src/lib/libdisasm/common/dis_sparc_fmt.c (Diff revisions 3 - 5)
     
     

    != 0, or > 0 please.

    1. the simple expression would be ASSERT0(bits == 0) - assert0 will fire if the value is not 0.

    2. Ah, okay, sure. :-)
      ... or change it to ASSERT() :-).
    3. Yes, please change it to ASSERT(). While ASSERT0() will work, it's not logically correct, given it's definition, and should be used to check for integer values being 0, not something being false.

  3. 
      
tsoome
marcel
  1. Ship It!
  2. 
      
tsoome
yuripv
  1. Ship It!
  2. 
      
vgusev
  1. 
      
  2. I would correct description to:

    • sign extend a val (that is 'bits' bits in length and signed bit is 'bits - 1') bit
    • to a 32-bit signed integer

    Or something like this. It is not obvious (by original comment) that this function does.

    1. Do not forget the purpose of the change is to correct the compiler warning;) I do not mind at all if someone will go for fixing the description, but I would really just stick on the intended change here and now.

  3. 
      
marcel
  1. Ship It!
  2. 
      
vgusev
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...