From: Coly Li <colyli@xxxxxxx>
> Sent: 12 February 2021 16:02
>
> On 2/12/21 11:31 PM, David Laight wrote:
> >>> if (c->gc_stats.in_use <= BCH_WRITEBACK_FRAGMENT_THRESHOLD_MID) {
> >>> - fp_term = dc->writeback_rate_fp_term_low *
> >>> + fp_term = (int64_t)dc->writeback_rate_fp_term_low *
> >>> (c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_LOW);
> >>> } else if (c->gc_stats.in_use <= BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH) {
> >>> - fp_term = dc->writeback_rate_fp_term_mid *
> >>> + fp_term = (int64_t)dc->writeback_rate_fp_term_mid *
> >>> (c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_MID);
> >>> } else {
> >>> - fp_term = dc->writeback_rate_fp_term_high *
> >>> + fp_term = (int64_t)dc->writeback_rate_fp_term_high *
> >>> (c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH);
> >>> }
> >>> fps = div_s64(dirty, dirty_buckets) * fp_term;
> >>>
> >>
> >> Hmm, should such thing be handled by compiler ? Otherwise this kind of
> >> potential overflow issue will be endless time to time.
> >>
> >> I am not a compiler expert, should we have to do such explicit type cast
> >> all the time ?
> >
>
> Hi David,
>
> I add Dongdong Tao Cced, who is author of this patch.
>
> Could you please offer me more information about the following lines?
> Let me ask more for my questions.
>
> > We do to get a 64bit product from two 32bit values.
> > An alternative for the above would be:
> > fp_term = c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH;
> > fp_term *= dc->writeback_rate_fp_term_high;
>
> The original line is,
> fp_term = dc->writeback_rate_fp_term_high * (c->gc_stats.in_use -
> BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH)
>
> The first value dc->writeback_rate_fp_term_high is unsigned int (32bit),
> and the second value (c->gc_stats.in_use -
> BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH) is unsigned int (32bit) too. And
> fp_term is 64bit, if the product is larger than 32bits, the compiler
> should know fp_term is 64bit and upgrade the product to 64bit.
>
> The above is just my guess, because I feel compiling should have the
> clue for the product upgrade to avoid overflow. But I almost know
> nothing about compiler internal ....
No, the expression is evaluated as 32bit and then extended for the assignment.
Or, more correctly, integer variables smaller than int (usually short and char)
are extended to int prior to any arithmetic.
If one argument to an operator is larger than int it is extended.
If there is a sign v unsigned mismatch the signed value is cast to unsigned
(same bit pattern on 2's compliment systems).
There are some oddities:
- 'unsigned char/short' gets converted to 'signed int' unless
char/short and int are the same size (which is allowed).
(Although if char and int are the same size there are issues
with the value for EOF.)
- (1 << 31) is a signed integer, it will get sign extended if used
to mask a 64bit value.
K&R C converted 'unsigned char' to 'unsigned int' - but the ANSI
standards body decided otherwise.
The compiler is allowed to use an 'as if' rule to use the 8bit and
16bit arithmetic/registers on x86.
But on almost everything else arithmetic on char/short local variables
requires the compiler repeatedly mask the value back to 8/16 bits.
The C language has some other oddities - that are allowed but never done.
(Except for 'thought-machines' in comp.lang.c)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
