1

Why is this warning appearing? It's not really an assumption if I check the bounds. And how to fix?

If num_actions_to_skip is set to 1, instead of 2, the error goes away.

Thanks

error: assuming signed overflow does not occur when assuming that (X - c) <= X is always true [-Werror=strict-overflow]
cc1plus: all warnings being treated as errors

On if (loc >= 0 && loc < action_list.count()) {

const QList<QAction *> &action_list = tool_menu->actions();
static const int num_actions_to_skip = 2;
const int loc = action_list.count() - num_actions_to_skip;
if (loc >= 0 && loc < action_list.count()) {
    tool_menu->insertAction(action_list.at(loc),
                            action);
}

It started with

Q_ASSERT_X(i >= 0 && i < p.size()

at qlist.h:454, which performs the same check, and throws this error as well, with just

tool_menu->insertAction(action_list.at(action_list.count() - 2),
                                action);
clark
  • 395
  • 5
  • 12

4 Answers4

9

You just need to rethink your logic.

static const int num_actions_to_skip = 2;
const int loc = action_list.count() - num_actions_to_skip;
if (loc >= 0 && loc < action_list.count()) {
    // ...
}

Apparently action_list.count() is a constant value (at least it won't change as this code is executed), and the compiler is able to figure that out.

Let's simplify this a bit, replacing num_actions_to_skip by 2, reducing action_list.count() to count. We can then re-express loc as count - 2.

Your if condition becomes:

if (count - 2 >= 0 && count - 2 < count)

which is equivalent (assuming, as the compiler warning said, that no overflow occurs) to:

if (count >= 2 && -2 < 0)

The second half of that, -2 > 0 is obviously true, so you can safely drop it, which leaves us with

if (count >= 2)

Re-substituting the original terms, this gives us:

static const int num_actions_to_skip = 2;
// const int loc = action_list.count() - num_actions_to_skip;
if (action_list.count() >= num_actions_to_skip) {
    // ...
}

The compiler warned you that it was performing an optimization that might be invalid if there's an integer overflow (it's permitted to assume that there is no overflow because if there is the behavior is undefined). It was kind enough to warn you about this -- which is lucky for you, because it pointed to the fact that your code is doing something it doesn't need to.

I don't know whether you need to keep the declaration of loc; it depends on whether you use it later. But if you simplify the code in the way I've suggested it should work the same way and be easier to read and understand.

If you get a warning message from the compiler, your goal should not just be to make the message go away; it should be to drill down and figure out just what the compiler is warning you about, and beyond that why your code causes that problem.

You know the context of this code better than I do. If you look at the revised version, you may well find that it expresses the intent more clearly.

Keith Thompson
  • 254,901
  • 44
  • 429
  • 631
  • I know that most of that is obvious, I was hoping that the compiler would think so too, and stop complaining. As my later edits say, the original problem was in a line of Qt code, which I attempted to cease with the explicit code in the original post. Thanks for the input – clark Aug 29 '13 at 23:40
  • misread originally. Note that `int(count - 2) >= 0` is district from `count - 2 >=0` -- there is no (defined) way an `int` produced from an unsigned can be negative, so that clause dies too. – Yakk - Adam Nevraumont Aug 30 '13 at 00:17
  • @Yakk: Yes, given that `count` is unsigned, you're right. But I think the OP's *intent* was to check that `count >= 2`. (Hard to tell without more context.) – Keith Thompson Aug 30 '13 at 00:47
5

From this GCC resource:

-Wstrict-overflow

-Wstrict-overflow=n

This option is only active when -fstrict-overflow is active. It warns about cases where the compiler optimizes based on the assumption that signed overflow does not occur. Note that it does not warn about all cases where the code might overflow: it only warns about cases where the compiler implements some optimization. Thus this warning depends on the optimization level.

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.

-Wstrict-overflow=1

Warn about cases that are both questionable and easy to avoid. For example, with -fstrict-overflow, the compiler simplifies x + 1 > x to 1. This level of -Wstrict-overflow is enabled by -Wall; higher levels are not, and must be explicitly requested.

-Wstrict-overflow=2

Also warn about other cases where a comparison is simplified to a constant. For example: abs (x) >= 0. This can only be simplified when -fstrict-overflow is in effect, because abs (INT_MIN) overflows to INT_MIN, which is less than zero. -Wstrict-overflow (with no level) is the same as -Wstrict-overflow=2.

-Wstrict-overflow=3

Also warn about other cases where a comparison is simplified. For example: x + 1 > 1 is simplified to x > 0.

-Wstrict-overflow=4

Also warn about other simplifications not covered by the above cases. For example: (x * 10) / 5 is simplified to x * 2.

-Wstrict-overflow=5

Also warn about cases where the compiler reduces the magnitude of a constant involved in a comparison. For example: x + 2 > y is simplified to x + 1 >= y. This is reported only at the highest warning level because this simplification applies to many comparisons, so this warning level gives a very large number of false positives.

BartoszKP
  • 34,786
  • 15
  • 102
  • 130
Buhake Sindi
  • 87,898
  • 29
  • 167
  • 228
  • I changed the data type to qreal(double), and the warning went away (no longer an int) perhaps the compiler was optimizing the int to an unsigned int. – clark Aug 29 '13 at 22:04
  • 2
    @foxrider67: No, a compiler isn't going to "optimize" an `int` to an `unsigned int`. The values you're working with are integers; changing them to `double` is not the answer. You need to rearrange your logic (I haven't studied this enough to know how). – Keith Thompson Aug 29 '13 at 22:47
1

The solution was to change my ints to unsigned ints:

const QList<QAction *> &action_list = tool_menu->actions();
static const unsigned int num_actions_to_skip = 2;
const unsigned int pos = action_list.count() - num_actions_to_skip;
assert(pos >= 0);
tool_menu->insertAction(action_list.at(pos),
                        action);
clark
  • 395
  • 5
  • 12
  • 5
    Eh, no. That is not the solution. It gets rid of the warning, because overflow on unsigned int is well-defined. If `count==1`, pos is still >=0. That's always the case, it is unsigned after all. But unsigned(1-2) is `UNIT_MAX-1`, very large. `at(pos)` will fail. – MSalters Aug 30 '13 at 08:05
0

I just solved a similar issue on a comparison that works fine for x86 but not for arm (optimization level O2): I replaced (x<right) by (x-right<0) and the code is fine now. And it sounds logical (on hindsight): The new code is worse to read for humans but expresses what the compiler does even for the overflow case.

static inline bool shape_int_rectangle_contains ( 
    const shape_int_rectangle_t *this_, int32_t x, int32_t y )
{
    bool result;
    const int32_t right = (*this_).left + (*this_).width;
    const int32_t bottom = (*this_).top + (*this_).height;
    /*
     * warning: assuming signed overflow does not occur when assuming that (X + c) >= X is always true [-Wstrict-overflow]
     *
     * result = ( x >= (*this_).left )&&( y >= (*this_).top )&&( x < right )&&( y < bottom );
     *
     * fix:
     */
    result = ( x-(*this_).left >= 0 )&&( y-(*this_).top >= 0 )&&( x-right < 0 )&&( y-bottom < 0 );
    return result;
}
anwan
  • 11
  • 3