8

Consider this code (bits.c):

#include <assert.h>
#include <inttypes.h>
#include <stdio.h>

static uint64_t pick_bits(unsigned char *bytes, size_t nbytes, int lo, int hi)
{
  assert(bytes != 0 && nbytes > 0 && nbytes <= 8);
  assert(lo >= 0 && lo < 64);
  assert(hi >= 0 && hi < 64 && hi >= lo);
  uint64_t result = 0;
  for (int i = nbytes - 1; i >= 0; i--)
    result = (result << 8) | bytes[i];
  result >>= lo;
  result &= (UINT64_C(1) << (hi - lo + 1)) - 1;
  return result;
}

int main(void)
{
  unsigned char d1[8] = "\xA5\xB4\xC3\xD2\xE1\xF0\x96\x87";
  for (int u = 0; u < 64; u += 4)
  {
    uint64_t v = pick_bits(d1, sizeof(d1), u, u+3);
    printf("Picking bits %2d..%2d gives 0x%" PRIX64 "\n", u, u+3, v);
  }
  return 0;
}

When compiled with stringent warnings (using GCC 4.8.2 built for an Ubuntu 12.04 derivative):

$ gcc -g -O3 -std=c99 -Wall -Wextra -Wmissing-prototypes -Wstrict-prototypes \
>     -Wold-style-definition -Wold-style-declaration -Werror  bits.c -o bits
In file included from bits.c:1:0:
bits.c: In function ‘main’:
bits.c:9:35: error: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Werror=strict-overflow]
   assert(hi >= 0 && hi < 64 && hi >= lo);
                                   ^
cc1: all warnings being treated as errors

I'm puzzled: how is GCC complaining about an addition? There are no additions in that line (even when preprocessed)! The relevant section of the preprocessed output is:

# 4 "bits.c" 2

static uint64_t pick_bits(unsigned char *bytes, size_t nbytes, int lo, int hi)
{
  ((bytes != 0 && nbytes > 0 && nbytes <= 8) ? (void) (0) : __assert_fail ("bytes != 0 && nbytes > 0 && nbytes <= 8", "bits.c", 7, __PRETTY_FUNCTION__));
  ((lo >= 0 && lo < 64) ? (void) (0) : __assert_fail ("lo >= 0 && lo < 64", "bits.c", 8, __PRETTY_FUNCTION__));
  ((hi >= 0 && hi < 64 && hi >= lo) ? (void) (0) : __assert_fail ("hi >= 0 && hi < 64 && hi >= lo", "bits.c", 9, __PRETTY_FUNCTION__));
  uint64_t result = 0;
  for (int i = nbytes - 1; i >= 0; i--)
    result = (result << 8) | bytes[i];
  result >>= lo;
  result &= (1UL << (hi - lo + 1)) - 1;
  return result;
}

Clearly, I can add -Wno-strict-overflow to suppress that warning, but I don't understand why the warning is considered to apply to this code in the first place.

(I note that the error is reputedly In function ‘main’:, but that is because it is able to aggressively inline the code of the function into main.)


Further observations

Some observations triggered by the answers:

  • The problem occurs because of the inlining.
  • Removing the static is not sufficient to avoid the problem.
  • Compiling the function separately from main works.
  • Adding the __attribute__((noinline)) works too.
  • Using -O2 optimization avoids the issue, too.

Subsidiary question

This looks to me like dubious behaviour by the GCC compiler.

  • Is it worth reporting to the GCC team as a possible bug?

Assembler output

Command:

$ gcc -g -O3 -std=c99 -Wall -Wextra -Wmissing-prototypes -Wstrict-prototypes \
> -Wold-style-definition -Wold-style-declaration -Werror -S \
> -Wno-strict-overflow bits.c
$

Assembler (top section):

    .file   "bits.c"
    .text
.Ltext0:
    .section    .rodata.str1.8,"aMS",@progbits,1
    .align 8
.LC0:
    .string "Picking bits %2d..%2d gives 0x%lX\n"
    .section    .text.startup,"ax",@progbits
    .p2align 4,,15
    .globl  main
    .type   main, @function
main:
.LFB8:
    .file 1 "bits.c"
    .loc 1 19 0
    .cfi_startproc
.LVL0:
    pushq   %rbp
    .cfi_def_cfa_offset 16
    .cfi_offset 6, -16
.LBB8:
.LBB9:
    .loc 1 23 0
    movl    $3, %edx
.LBB10:
.LBB11:
    .loc 1 13 0
    movabsq $-8676482779388332891, %rbp
.LBE11:
.LBE10:
.LBE9:
.LBE8:
    .loc 1 19 0
    pushq   %rbx
    .cfi_def_cfa_offset 24
    .cfi_offset 3, -24
.LBB22:
    .loc 1 21 0
    xorl    %ebx, %ebx
.LBE22:
    .loc 1 19 0
    subq    $8, %rsp
    .cfi_def_cfa_offset 32
    jmp .L2
.LVL1:
    .p2align 4,,10
    .p2align 3
.L3:
    leal    3(%rbx), %edx
.LVL2:
.L2:
.LBB23:
.LBB20:
.LBB16:
.LBB12:
    .loc 1 13 0
    movl    %ebx, %ecx
    movq    %rbp, %rax
.LBE12:
.LBE16:
    .loc 1 24 0
    movl    %ebx, %esi
.LBB17:
.LBB13:
    .loc 1 13 0
    shrq    %cl, %rax
.LBE13:
.LBE17:
    .loc 1 24 0
    movl    $.LC0, %edi
.LBE20:
    .loc 1 21 0
    addl    $4, %ebx
.LVL3:
.LBB21:
.LBB18:
.LBB14:
    .loc 1 13 0
    movq    %rax, %rcx
.LBE14:
.LBE18:
    .loc 1 24 0
    xorl    %eax, %eax
.LBB19:
.LBB15:
    .loc 1 14 0
    andl    $15, %ecx
.LBE15:
.LBE19:
    .loc 1 24 0
    call    printf
.LVL4:
.LBE21:
    .loc 1 21 0
    cmpl    $64, %ebx
    jne .L3
.LBE23:
    .loc 1 27 0
    addq    $8, %rsp
    .cfi_def_cfa_offset 24
    xorl    %eax, %eax
    popq    %rbx
    .cfi_def_cfa_offset 16
.LVL5:
    popq    %rbp
    .cfi_def_cfa_offset 8
    ret
    .cfi_endproc
.LFE8:
    .size   main, .-main
    .text
...
Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
  • Can you share the generated assembly, too? – Carl Norum Apr 11 '14 at 18:42
  • Compiling this under clang, I get no warnings or errors. – mah Apr 11 '14 at 18:42
  • "Is it worth reporting to the GCC team as a possible bug?", what do you consider as a bug? GCC gives an error because you asked for all warnings to be considered as errors. It warns because it is generating code which assumes no overflow takes place. There is a way to disable that warning if you don't want it. – AProgrammer Apr 11 '14 at 19:23
  • Alternative: `assert(hi >= 0 && hi < 64); assert(lo >= 0 && lo <= hi);`. I do not think the issue is about `u+3`, but that the 2 asserts have 5 tests when 4 would do. – chux - Reinstate Monica Apr 11 '14 at 19:24
  • @AProgrammer: what I think is an error is that GCC hasn't noticed that the overflow cannot occur because the values are so constrained that even if the arithmetic were performed using `int8_t`, the addition of 3 would never overflow `lo + 3`. – Jonathan Leffler Apr 11 '14 at 19:30
  • @chux: Unfortunately, your suggested change doesn't alter the result. – Jonathan Leffler Apr 11 '14 at 19:31
  • Even more simple `assert(lo >= 0 && hi < 64 && lo <= hi);` – chux - Reinstate Monica Apr 11 '14 at 19:31
  • @chux: Sadly, the single assertion checking `lo` and `hi` does not alter the result either; it still generates the error. – Jonathan Leffler Apr 11 '14 at 19:33
  • I tried. [No soup for me](http://en.wikipedia.org/wiki/The_Soup_Nazi) – chux - Reinstate Monica Apr 11 '14 at 19:42
  • @JonathanLeffler, I see. They have looked at the root cause of similar reports and your case doesn't seem a duplicate of them. See for instance http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52904. So yes, that could be worthwhile. – AProgrammer Apr 11 '14 at 20:17
  • Consider this warning a good one. It tells you that the types of your interface are wrong. Using signed types for `lo` and `hi` just complicates the reasoning for the compiler. Conceptually these are unsigned quantities, so use `unsigned` for them. – Jens Gustedt Apr 11 '14 at 20:57
  • @JensGustedt: When using `unsigned` in place of `int`, and remembering to remove the assertions about `>= 0`, adjust the loop in `pick_bits()` so that it does not underflow 0 to a large value, and fix the `printf()` format string -- with the necessary changes made, the warning is indeed eliminated. – Jonathan Leffler Apr 11 '14 at 21:36
  • For the downward I loop, I personally simply would go for something like `for (size_t i = nbytes - 1; i < nbytes; i--)`. – Jens Gustedt Apr 12 '14 at 08:58
  • I think the `lo=0, hi=63` case is UB since it shifts a 64 bit value by 64 – CodesInChaos Aug 04 '14 at 11:50
  • @CodesInChaos: yes, you're right that the code invokes UB when `lo=0, hi=63`. The fix for that would be to make the final two assignment operations in the function conditional on `if (hi - lo < 64) { result >>= lo; result &= (UINT64_C(1) << (hi - lo + 1)) - 1; }`. I think that is tangential to the compiler warning, though — fixing it does not alter the warning (and neither does using GCC 4.9.1). – Jonathan Leffler Aug 04 '14 at 15:12

2 Answers2

7

It's inlining the function, and then generating the error. You can see for yourself:

__attribute__((noinline))
static uint64_t pick_bits(unsigned char *bytes, size_t nbytes, int lo, int hi)

On my system, the original version generates the same warning, but the noinline version does not.

GCC then optimizes out hi >= lo, because it's really u+3 >= u, and generates a warning because it's not good enough at figuring out that u+3 doesn't overflow. A shame.

Documentation

From the GCC documentation, section 3.8:

An optimization that assumes that signed overflow does not occur is perfectly safe if the values of the variables involved are such that overflow never does, in fact, occur. Therefore this warning can easily give a false positive: a warning about code that is not actually a problem. To help focus on important issues, several warning levels are defined. No warnings are issued for the use of undefined signed overflow when estimating how many iterations a loop requires, in particular when determining whether a loop will be executed at all.

Emphasis added. My personal recommendation is to use -Wno-error=strict-overflow, or to use a #pragma to disable the warning in the offending code.

Dietrich Epp
  • 205,541
  • 37
  • 345
  • 415
  • Removing the `static` is not sufficient. Compiling the function separately from `main` works, and adding the `__attribute__((noinline))` works too. Using `-O2` optimization avoids the issue, too. Subsidiary question: is it worth reporting to the GCC team as a bug? – Jonathan Leffler Apr 11 '14 at 18:57
  • 1
    The worst that will happen is that they will tell you that false positives are inevitable, and you should ignore it or turn it off. The fact that this warning is often a false positive is written right into the GCC docs. I lean towards turning the warning off, selectively. Static analysis is hard. – Dietrich Epp Apr 11 '14 at 20:16
  • I'm puzzled by two points: 1. Of course by inlining the compiler sees more than I do, but why locate the warning in the assert in pick_bits(), instead of at the call site where the addition really occurs? (pick_bits(d1, sizeof(d1), u, u+3);). 2. The author of pick_bits isn't going to remove the sensible assert just because of an issue at the call site. To suppress the error with pragmas takes the ridiculous #ifdef __GNUC__ #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wstrict-overflow" source line #pragma GCC diagnostic pop #endif – pdbj Sep 12 '16 at 19:41
  • [arg, line breaks stripped in comments] Really? Is that the only way to use strict-overflow to find actual problems? Litter the code with five lines of pre-processor for issues raised by the caller? – pdbj Sep 12 '16 at 19:42
  • @pdbj: Undefined overflow is generally a mess, C is generally a mess, and in this messy environment, it is inevitable that you will get false positive warnings unless you go to extraordinary lengths to avoid them. The reason why the warning is generated inside `pick_bits` is because it is the code in `pick_bits` which triggers the error: `hi >= lo` is the triggering code. The definition `hi = lo + 3` is part of the context. I think if you want to avoid `#ifdef` altogether you would have to give up on either C or portability, unless your programs are simple. – Dietrich Epp Sep 12 '16 at 19:54
  • @pdbj: Maybe at some point in the future, GCC will be able to identify the pieces of context that allowed it to deduce that `hi >= low` can be optimized out. The Clang static analyzer can do things like this. – Dietrich Epp Sep 12 '16 at 19:55
6

My assumption is that gcc is inlining pick_bits and thus compiling with the knowledge that hi == lo+3 which allows it to assume that hi >= lo is always true as long as lo is low enough that lo+3 doesn't overflow.

AProgrammer
  • 51,233
  • 8
  • 91
  • 143
  • +1, this was my intuition, too. I'd guess aggressive inlining is included with `-O3`. – Carl Norum Apr 11 '14 at 18:44
  • 4
    Technically, it's allowed to assume that `lo+3` doesn't overflow because the C standard says so. Even if `lo = INT_MAX`. – Dietrich Epp Apr 11 '14 at 18:50
  • @DietrichEpp, Agreed, gcc probably warns that the code assume no overflow, and the user asked gcc to consider all warnings as errors. – AProgrammer Apr 11 '14 at 19:19