You are not logged in.
Hi all. A somewhat recent gcc upgrade introduced the "nonnull" attribute to
`this` pointers of C++ member functions, which broke some programs. I have a
few fixes below, but none of them feel right. Wondering if anyone knows a
better fix. I'd also welcome your thoughts on whether or not this is a
legitimate issue with gcc, or if I should just learn to program better.
For those with enough patience, here's a minimal example:
/* test.cpp: demonstrate potentially valid use of a null this pointer. */
#include <cstdio>
#include <cstdlib>
class node
{
public:
node* left;
node* right;
int data;
/* NOTE: toggling optimization for this function prevents the bug. */
// void __attribute__((optimize("O0"))) postOrder()
void postOrder()
{
/* NOTE: casting to void* prevents the warning, but not the bug. */
if (this == 0) return;
left->postOrder();
right->postOrder();
printf("%i ",this->data);
}
};
int main(int argc, char *argv[])
{
size_t k = (argc>1)?(atoi(argv[1])):15; /* # nodes */
node* T = new node[k]; /* allocate nodes */
for (size_t i = 0; i < k; i++) { /* connect pointers, set data... */
T[i].left = (2*i+1<k)?(T+2*i+1):0;
T[i].right = (2*(i+1)<k)?(T+2*(i+1)):0;
T[i].data = i;
}
T[0].postOrder();
printf("\n");
return 0;
}Compile with `g++ -O2 test.cpp` and (for me anyway) `./a.out 19` will crash.
Fixes I've found:
1. Don't optimize that bit of code (see the comments above `postOrder`).
2. Write the function differently (stop recursion one frame above).
Concerns / why I don't like either of these fixes:
1. I did my best to read the relevant sections of the C++ standard (e.g. 9.3.2
on "the this pointer" from the current working draft), and it never
mentions that `this` pointers can't be null.
2. The warning is included in -Wall (you'll get -Wnonnull-compare), but
casting the pointer can remove the warning and preserve the bug.
3. I can't find precisely which optimization to turn off to avoid this, nor
can I figure out how to undo the nonnull attribute on a member function.
Furthermore, whatever optimization was turned on is implemented strangely,
making debugging difficult.^[a]
I know it seems a minor point, but I think one of gcc's goals is to faithfully
preserve the functionality of programs that are technically correct (even for
programs with questionable style).
Thanks in advance for your help!
Footnotes
a. In particular, the check isn't always removed. The recursion is pretty
heavily inlined with -O2, and the check against null is removed only on the
outermost frame, if that makes sense. (This is why you have to make a tree
of 19 things before it crashes.)
Last edited by wes (2017-01-28 22:18:36)
Offline
This may be helpful (from here: http://stackoverflow.com/questions/1844 … s-is-null).
Regarding it making sense to check for `this == NULL`:
In standard C++, it does not, because any call on a null pointer is already undefined behavior, so any code relying on such checks is non-standard (there's no guarantee that the check will even be executed).
Further, regarding `this` being null:
It means that a method was called on a null pointer, or on a reference obtained from a null pointer (though obtaining such a reference is already U.B.). This has nothing to do with delete, and does not require any objects of this type to have ever existed.
Regarding your postOrder() function: if your left/right pointers can be invalid, you should rather check that they are valid before using them.
Edit:
To maybe add more clarity: the problem shouldn't (as far as I know) really be with the `if (this == 0)` check, though as the post I linked to explains, it doesn't really make sense. The problem is that you have an instance of a `node` where the value of either the `left` or `right` (or both) member variable is NULL (I *think* your node objects are value-initialized), and you attempt to call postOrder() on it.
Last edited by aoba (2017-01-28 05:54:59)
Offline
Thanks! Your link encouraged me to keep googling, and I was able to find in
the standard (in a roundabout way), that indeed this is undefined behavior.
Here's another link I thought was helpful if others happen upon this later:
http://stackoverflow.com/questions/2474 … fined-beha
In my brain, `p->function()` was distinct from `p->variable` since they are
very different behind the scenes: the former doesn't necessarily result in
reading *anything* from memory at (or near) `p`, but the latter does. The
reasoning in the standard does not account for this difference. They argue as
follows: `p->thing` reduces to `(*p).thing`, and if `p` is null, `*p` is
always undefined. Hence `p->function()` is invalid if `p==0`. Fine.
Anyway, I'm marking as solved. Thanks for your help!
Offline