Notes |
(0010854)
vda
08-28-08 13:53
|
long is always signed in C. Only char has unspecified signedness.
Please give a concrete example of incorrect awk behavior. If GNU awk produces a result which is different from busybox's awk, that is a good indication of a bug. |
| |
(0010864)
vda
08-28-08 16:16
|
Applied to svn, thanks! Also see:
http://busybox.net/downloads/fixes-1.12.0/busybox-1.12.0-awk.patch [^] |
| |
(0010914)
benoar
08-31-08 20:41
|
I reopen this bug after I made some more investigation on the root cause of the problem, and to let busybox developers decide what is the right thing to do.
First, vda, sorry for the signedness confusion : you are right, long is always signed, there is no such thing as "default signedness" for it, and the problem doesn't come from there. This is where I was confused.
Actually, the "problem" comes from the cast from double (internal bb awk representation of numbers) to (unsigned) long. The bug I saw in openwrt was in fact that different architecture casted to different integral values for values > 2^31-1.
But I think the reason for this behavior is that there is no rule on how to cast a value _not_ in the destination's type range, as 0x80000000 > LONG_MAX ! So, the behavior was undefined, and I got different results on different archs. This is only a supposition, please correct me if I am wrong.
So, the patch I sent obviously matches what suits me, but I am not sure it pleases everybody : now, negative values in bitwise operations get cast to something undefined. I get 0 on an armeb for example.
To me, using bitwise ops on values >0 and <ULONG_MAX looks good, as opposed to values >LONG_MIN and <LONG_MAX. But not everybody may agree. If someone can find some reference on awk's behavior on integers limits regarding bitwise ops, I'd be gratefull. |
| |
(0010924)
vda
09-02-08 01:59
|
Try attached 5.patch, it should fix it |
| |
(0010934)
benoar
09-02-08 09:41
|
What ? It's not by doing some magic casting that you will solve it. I get the same result as before with your patch, as it eventually gets casted to unsigned long (the return type of your getvar_i_int function). Furthermore, this looks far uglier than the previous solution.
If you really want to "extend" the range of bitwise functions, you will have to create a macro or something to do operations on both types (signed and unsigned longs). But then, what do you do when the two operands aren't of the same type ?...
I think the best solution is to choose wether we use signed or unsigned longs. Not extending it.
BTW, your octal test fails on my ppc (native) and armeb (cross-compiled), both giving 1235 as a result. |
| |
(0010944)
vda
09-03-08 10:36
|
> It's not by doing some magic casting that you will solve it.
+ /* Casting doubles to longs is undefined for values outside
+ * of target type range. Try to widen it as much as possible */
+ if (d >= 0)
+ return (unsigned long)d;
Above will correctly handle any value in [0.0, UINT_LONG]
+ return - (long) (unsigned long) (-d);
and if d < 0, we convert (-inf, 0.0) to (0.0, inf) by inverting the sign, then if it is in (0.0, UINT_LONG], we convert it to unsigned long. Then we invert it again to compensate for (-d).
Which step does not work for you? Can you add
bb_error_msg("1 step: %f", -d);
bb_error_msg("2 step: %ul", (unsigned long) -d);
etc? |
| |
(0011024)
benoar
09-04-08 13:00
|
> + /* Casting doubles to longs is undefined for values outside
> + * of target type range. Try to widen it as much as possible */
> + if (d >= 0)
> + return (unsigned long)d;
>
> Above will correctly handle any value in [0.0, UINT_LONG]
Correct.
> + return - (long) (unsigned long) (-d);
>
> and if d < 0, we convert (-inf, 0.0) to (0.0, inf) by inverting the sign, then if it is in (0.0, UINT_LONG], we convert it to unsigned long. Then we invert it again to compensate for (-d).
You just forget one last step : the result get implicitly cast to the return type of your function, which is unsigned long ; and the value you're catsing is out of its range ! (you negate a positive value, so it will be < 0). Furthermore, casting integral value like that doesn't change anything : signed/unsigned long's representation doesn't get changed when casted to the same kind of type (here, 32bits integral value -- on 32bits arch, that is). But here, as you're casting a long to an integral type which is of the "same kind", the result is not "undefined", it's just let "as is", which is what we want with bitwise operations. Please note that all what I said is not verified, I am not a compiler internals guru, but it looks fine to me.
So, your patch effectively extends the range, and the useful bit in it is that the double gets cast to something meaningful for the destination type : here, some positive number for unsigned long. But we could do simpler : just cast the value to a long (where the FP to integral type conversion takes place), and then let it be casted to unsigned long (the base type of our bitwise operations). No need for a double negation. Patch is attached (diff to the current svn).
Sorry for the too quick look at first on your patch. |
| |
(0011034)
vda
09-04-08 13:46
edited on: 09-04-08 13:48
|
> the result get implicitly cast to the return type of your function, which is unsigned long ; and the value you're catsing is out of its range ! (you negate a positive value, so it will be < 0).
But for unsigned long <-> long, unlike double -> long, the cast is basically a no-op, bit image of the value is not changed. For example, -2L can be cast to unsigned long and it will have a value of 0xfff...fffe. "Out of range" clause is not applicable, unlike the case where you cast double -> long.
> Furthermore, casting integral value like that doesn't change anything : signed/unsigned long's representation doesn't get changed when casted to the same kind of type (here, 32bits integral value -- on 32bits arch, that is).
Exactly. That's what I wanted. My (long) cast there is just to avoid having an unary minus applied to unsigned value, which feels fishy. Then it is cast back to unsigned long (implicitly).
> But we could do simpler : just cast the value to a long (where the FP to integral type conversion takes place)
This will fail for large values. For 32-bit longs, 0xffffffff will become a double value of 4294967295.0 and then will be cast to long -> out of range -> undefined! Not good. (It may work on some arches, but it is purely by chance. I want more robust solution.)
If 5.patch doesn't work for you, can you find out why? Can you add
bb_error_msg("1 step: %f", -d);
bb_error_msg("2 step: %ul", (unsigned long) -d);
bb_error_msg("3 step: %l", (long)(unsigned long) -d);
bb_error_msg("4 step: %ul", (unsigned long) (- (long)(unsigned long) -d) );
etc?
|
| |
(0011044)
vda
09-04-08 13:53
|
Your patch:
if (d >= 0)
return (unsigned long)d;
- return - (long) (unsigned long) (-d);
+ return (long)d;
Let's say d == -4294967295.0
It's (- 0xffffffff), or just 1 if longs are 32-bit. Above code would not handle it correctly. |
| |
(0011064)
benoar
09-04-08 16:46
|
> This will fail for large values. For 32-bit longs, 0xffffffff will become a double value of 4294967295.0 and then will be cast to long -> out of range -> undefined! Not good. (It may work on some arches, but it is purely by chance. I want more robust solution.)
No, because we first test if value is >0, then we cast it to unsigned long ...
> If 5.patch doesn't work for you, can you find out why? Can you add
OK, I'm sorry for my previous answer, your patch actually work on my armeb target (and my ppc too, but it already worked with my previous patch, contrary to the armeb). I may have mixed up all the patches ...
> It's (- 0xffffffff), or just 1 if longs are 32-bit. Above code would not handle it correctly.
Well, I was thinking that by "extending", you meant from LONG_MIN to ULONG_MAX, not to something 32 bits archs don't have representation for (i.e. values < LONG_MIN)! I you want to handle values <LONG_MIN and >(-ULONG_MAX), why not, but to me it doesn't mean a lot to programmers as 32bits archs don't have integral representation for values this low.
So, I am ok with your patch, just that it looks a bit strange when you first look at it.
Thanks for the analysis, though, this made me think a bit more about all this ! |
| |
(0011174)
vda
09-08-08 11:05
|
fixed in svn. |
| |
(0011204)
benoar
09-08-08 14:30
|
I am sorry but I still don't get it for values < LONG_MIN. They don't have a representation, neither on 32 bits and 64 bits machines (as LONG_MIN is then adapted).
I may sound pedantic about willing to avoid a double negation, but as it gives totally meaningless results, I don't get the point.
In your example, -(-4294967295.0) get cast to an unsigned long (=4294967295), where it will be represented as 0xffffffff, then it's cast to a long (no-op) where it will be -1, and then negated again to get 1 ... what does it mean ? There are no representation for such number with integral values.
If you will get something meaningless (for values < MIN_LONG, that is), why not let the direct cast to long for negative values, and avoid some useless calculation, that looks iffy, indeed ?
Otherwise, give me an example where doing bitwise operations on these values has some meaning.
If you feel ok with it, let it be, but someone who will review the code won't understand it, I think. |
| |
(0011314)
vda
09-10-08 06:36
|
What is, in your opinion, the "most correct output" for awk in this case, assuming that awk uses 32-bit math for integers?
echo | awk '{ print or(-4294967295,1) }' |
| |
(0011414)
benoar
09-11-08 07:31
|
IMHO, there is no "most correct output" for this operation, on 32bits. The result is undefined. What would you like it to be ? I really don't understand.
BTW, running your example, gawk gives me "2,14748e+09" on 32bits ppc and "9007194959773697" on amd64, so neither get it "right", as both give positive numbers and I think you would like them to return is at least some negative one ? |
| |
(0011434)
vda
09-11-08 12:07
|
> IMHO, there is no "most correct output" for this operation, on 32bits. The result is undefined.
What result should it print? It should print SOMETHING, right? In your opinion, what should it print, and why?
In my opinion, it should print "1", because -4294967295 in two's complement representation is fff...fff00000001 and low 32 bits contain 00000001. (1 | 1) = 1. |
| |
(0011444)
benoar
09-11-08 16:20
|
> > IMHO, there is no "most correct output" for this operation, on 32bits. The result is undefined.
> What result should it print? It should print SOMETHING, right? In your opinion, what should it print, and why?
It should print what a direct cast to a long will give, which is something, even if it's undefined. No need to fiddle more this undefined value to get a more undefined one ...
> In my opinion, it should print "1", because -4294967295 in two's complement representation is fff...fff00000001 and low 32 bits contain 00000001. (1 | 1) = 1.
Well, these are 64bits considerations, which, to me, are meaningless on 32bits. This may be where we don't agree.
For example, what would you want -((2^64)-1) to be on 64bits archs ? The low bits of the two's complement of the 128bits representation ? ... |
| |
(0011454)
vda
09-13-08 04:39
|
>> In my opinion, it should print "1", because -4294967295 in two's complement representation is fff...fff00000001 and low 32 bits contain 00000001. (1 | 1) = 1.
>Well, these are 64bits considerations
No, these are infinite-bit considerations.
>It should print what a direct cast to a long will give
I don't want to return to this problem AGAIN when someone who disagrees with you will stumble over this. I'd rather proactively make it work, even if this case is somewhat unlikely. Especially considering that on x86, code size difference is 2 bytes only. |
| |
(0011474)
benoar
09-15-08 14:53
|
OK, I won't argue anymore, because my assertions are not based on technical fact (I don't care about code size, useless operations, ...) but only on "what one would think to be the most logical" which, as you point out, is programmer-dependent.
I just find it ugly, but, hey, it works.
Thanks for the different view on it, though. |
| |
(0011664)
vda
09-19-08 13:25
|
Fixed in svn. |
| |
bitwise_ops_on_unsigned_long.patch [^] (1,224 bytes) 08-27-08 21:33
