[optional] Strict Aliasing Warnings on Trunk
I've recently been seeing this error more and more especially on GCC with -Wall: /home/dean/boost/boost/optional/optional.hpp:407: warning: dereferencing pointer ‘<anonymous>’ does break strict-aliasing rules /home/dean/boost/boost/optional/optional.hpp:427: note: initialized from here and: /home/dean/boost/boost/function/function_base.hpp:321: warning: dereferencing type-punned pointer will break strict-aliasing rules /home/dean/boost/boost/function/function_base.hpp:325: warning: dereferencing type-punned pointer will break strict-aliasing rules This is on Linux and GCC 4.4.1. I'm not an expert in this regard but is there a way to either silence or avoid these warnings? Anything you want me to try from my end? Thanks in advance and I hope this helps. -- Dean Michael Berris blog.cplusplus-soup.com | twitter.com/mikhailberis linkedin.com/in/mikhailberis | facebook.com/dean.berris | deanberris.com
Mathias Gaunard wrote:
Using a reinterpret_cast to convert from one pointer type to an unrelated type seems to conflict with the rules at the end of chapter 3 of the standard. "If a program attempts to access the stored value of an object through an lvalue of other than one of the following types the behavior is undefined: ..." The gcc 4.4 documentation recommends using a union, instead of a type punned pointer or reference. Bo Persson
Bo Persson wrote:
It is not a trivial problem and not uncommon at the same time. Perhaps it would be a good idea if Boost provides some (more or less) portable guidelines on the subject of strict aliasing issues. On Wiki? The most complete reference I've found so far is this: http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-al... Best regards, -- Mateusz Loskot, http://mateusz.loskot.net
On Thu, Dec 17, 2009 at 5:51 PM, Bo Persson <bop@gmb.dk> wrote:
the conversion itself does not conflict with the standard. It is the dereferencing of the pointer that migh...
[this list include accessing an object with a type different from its dynamic type] ... but the dynamic type of the internal buffer of boost::optional has been changed via placement new, so (modulo compiler bugs) it should be safe: at any time the buffer has a specific type (when initialized) or no type at all (it is just a bunch of memory). Boost optional only dereferences the buffer when it has been initialized, and always with the same type used for placement new. Of course this is hard (impossible?) to prove statically and gcc warns even if it should be perfectly safe . HTH, -- gpd
On Thu, Dec 17, 2009 at 10:07 AM, Giovanni Piero Deretta <gpderetta@gmail.com> wrote:
We had a discussion about warnings some time ago, I think this is an excellent example of a warning that warns about something important yet it is completely useless. To someone who doesn't know the semantics of reinterpret_cast, perhaps the warning has some value, but for the rest of us what it really says is "Warning! You have used reinterpret_cast!" Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode
On Fri, Dec 18, 2009 at 3:10 AM, Emil Dotchevski <emildotchevski@gmail.com> wrote:
I don't understand why it's "completely useless" when in cases where they are done wrong it can cause a runtime crash.
Sure, but isn't reinterpret_cast considered bad because of the fact that there is no standard implementation of it? I don't see why the warning doesn't help you know that there is something *potentially* wrong with the implementation. I still have not seen any other concrete ways of avoiding (or silencing) this warning at the code level. Maybe some pragmas to disable the warning if we're all really convinced this is a spurious warning? -- Dean Michael Berris blog.cplusplus-soup.com | twitter.com/mikhailberis linkedin.com/in/mikhailberis | facebook.com/dean.berris | deanberris.com
On Thu, Dec 17, 2009 at 12:19 PM, Dean Michael Berris <mikhailberis@gmail.com> wrote:
Because the information the compiler gives you in this case is "you have used reinterpret_cast, your program *might* violate the C++ standard" but it can't know if you're violating it or not. So what it really says is "warning, you're using reinterpret_cast" which is useless because you know that you are using reinterpret_cast.
I'm not sure what you mean. There is a standard implementation of reinterpret_cast. Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode
On Fri, Dec 18, 2009 at 9:16 AM, Emil Dotchevski <emildotchevski@gmail.com> wrote:
Are we still talking about the strict aliasing rules? Because the strict aliasing stuff doesn't happen only where reinterpret_cast is done. In the case of Boost.Optional and GCC 4.4 (which apparently has a bug specific to this case) it's saying accessing a certain area in memory whose pointer is cast to be a pointer to a type which might not be related to the original type of the data in that area in memory *may* cause a runtime error. This is one of the reasons why I really don't like pointers in C/C++ -- the fact that its type can be modified in a standard manner makes it really really dangerous. But then that's a completely different matter altogether. ;)
Really? I thought this was implementation-defined? If you mean 'static_cast<foo*>(static_cast<void*>(bar_ptr))' I don't think it's guaranteed that that's really how reinterpret_cast is implemented by compilers. -- Dean Michael Berris blog.cplusplus-soup.com | twitter.com/mikhailberis linkedin.com/in/mikhailberis | facebook.com/dean.berris | deanberris.com
On Thu, Dec 17, 2009 at 11:17 PM, Dean Michael Berris <mikhailberis@gmail.com> wrote:
Sure, but it is specified that if you reinterpret_cast from T1* to T2*, then reinterpret_cast the resulting T2* back to T1*, you'll get the original value (as long as T2 doesn't have stricter alignment requirements.) Reinterpret_cast is tricky but it is part of the language and it's lame for the compiler to warn when it is used. Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode
On Fri, Dec 18, 2009 at 4:11 PM, Emil Dotchevski <emildotchevski@gmail.com> wrote:
See, the strict alignment requirements are what the compiler is warning about -- saying that it *may* break as a diagnostic is how it does it. Because the issue raised has already been pointed out to be a bug in that it's almost indiscriminate that even legitimate constructs do get flagged as potentially breaking strict aliasing rules, I'd say reinterpret_cast<> aside, the strict aliasing warnings do have merit in situations that they may cause problems at runtime. It's not *only* in cases that you have reinterpret_cast that it warns -- heck, it can even warn for using C-style casts and not only in C++, but even in C mode.
Reinterpret_cast is tricky but it is part of the language and it's lame for the compiler to warn when it is used.
Which warning are you pointing at? The strict aliasing warnings? Again, it's not only in cases where you have reinterpret_cast that it warns, it also happens in cases where you do cast the types of the pointers and try to dereference these type-punned or casted pointers. What *is* lame is how after years of references and legitimate safe pointer operations not involving reinterpret_cast, we're still having a discussion about it -- reinterpret_cast. ;) -- Dean Michael Berris blog.cplusplus-soup.com | twitter.com/mikhailberis linkedin.com/in/mikhailberis | facebook.com/dean.berris | deanberris.com
On Fri, Dec 18, 2009 at 6:11 AM, Emil Dotchevski <emildotchevski@gmail.com> wrote:
On Thu, Dec 17, 2009 at 11:17 PM, Dean Michael Berris <mikhailberis@gmail.com> wrote:
[snip]
But nothing else is guaranteed. The compiler might very well increment it by one byte when casting to T2 and then decrement it when casting it back. It is perfectly valid behavior for reinterpret_cast. IMO we should be using static_cast<T2*>(static_cast<void*>(x)) which is not implementation-defined.
-- Felipe Magno de Almeida
Felipe Magno de Almeida wrote:
Maybe, in principle, but if you try to write such a compiler you'll find that it isn't as easy, even if we dismiss C compatibility as a concern and stick to C++03. (In the latest C++0x draft reinterpret_cast for pointers to standard layout types is defined to do the static_cast dance.)
IMO we should be using static_cast<T2*>(static_cast<void*>(x)) which is not implementation-defined.
There isn't much difference. In practice, the two do the same thing. g++ should issue the same warning for both; it doesn't because the current frontend isn't smart enough to see through the intermediate void*.
On Fri, Dec 18, 2009 at 12:21 PM, Peter Dimov <pdimov@pdimov.com> wrote:
Felipe Magno de Almeida wrote:
[snip]
Would be really strange to write such compiler. But we don't have any reason to use reinterpret_cast. The only reason I can think of is typing less, and that's not a very compelling argument.
There is IMO. I know it is not related to the warning problem in gcc though. But one uses implementation-defined behavior, the other doesn't. I don't see any reason why we should prefer the implementation-defined behavior. -- Felipe Magno de Almeida
On Sat, Dec 19, 2009 at 5:11 AM, Patrick Horgan <phorgan1@gmail.com> wrote:
Are you using GCC 4.4.1? Because I'm pretty sure having -Wall turned on with GCC 4.4 on Boost.Trunk would yield the warning. -- Dean Michael Berris blog.cplusplus-soup.com | twitter.com/mikhailberis linkedin.com/in/mikhailberis | facebook.com/dean.berris | deanberris.com
Dean Michael Berris wrote:
Yep, with 4.4.1 and with 4.5 and -Wall, and optimization, or alternatively with -fstrict-aliasing -Wstrict-aliasing=3 the following code had no warnings at all with boost-trunk: #include <boost/optional.hpp> char foo() { return *boost::optional<char>('c'); } int main() { char c; c=foo(); }
On Fri, Dec 18, 2009 at 5:46 AM, Felipe Magno de Almeida <felipe.m.almeida@gmail.com> wrote:
It is perfectly portable as well.
IMO we should be using static_cast<T2*>(static_cast<void*>(x)) which is not implementation-defined.
Casting to a pointer of unrelated type doesn't suite static_cast, reinterpret_cast is a better choice. Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode
On Fri, Dec 18, 2009 at 4:57 PM, Emil Dotchevski <emildotchevski@gmail.com> wrote:
On Fri, Dec 18, 2009 at 5:46 AM, Felipe Magno de Almeida <felipe.m.almeida@gmail.com> wrote:
[snip]
It is not portable if we dereference it, which optional does.
I don't understand why. How is reinterpret_cast better than static_cast?
-- Felipe Magno de Almeida
On Fri, Dec 18, 2009 at 11:07 AM, Felipe Magno de Almeida <felipe.m.almeida@gmail.com> wrote:
I am going off of what Giovanni said earlier in this thread about how optional works, which seems to contradict your statement above.
In this case reinterpret_cast is a better choice because even with static_cast, the cast is just as unsafe, but may appear safer because static_cast is perceived as safer than reinterpret_cast. You don't want to make it harder for the programmer to see that the operation is potentially dangerous, do you? Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode
On Fri, Dec 18, 2009 at 6:19 PM, Emil Dotchevski <emildotchevski@gmail.com> wrote:
On Fri, Dec 18, 2009 at 11:07 AM, Felipe Magno de Almeida <felipe.m.almeida@gmail.com> wrote:
[snip]
I re-read what Giovanni said. And it seems to me it still dereferences it. Indirectly, but does it. If a perfectly valid implementation negates the bits in a a pointer for a reintepret_cast and you pass it to placement new it will probably fail. [snip]
It is not just as unsafe. static_cast is guaranteed to do what is needed. reinterpret_cast doesn't. It is implementation-defined and can do whatever it wants, as long as a reinterpret_cast back results in the same pointer as before.
You don't want to make it harder for the programmer to see that the operation is potentially dangerous, do you?
It is if you do reintepret_cast. Not if you do static_cast.
[snip] -- Felipe Magno de Almeida
Just a thought about why we might not like implementation defined behavior. It's hard to get things to work with all the compilers without delving into implementation defined areas. Patrick
On Fri, Dec 18, 2009 at 9:19 PM, Emil Dotchevski <emildotchevski@gmail.com> wrote:
Just to clarify, my comment was only about aliasing issues, i.e. in my opinion boost.optional has none. I didn't comment on boost.optional usage of reinterpret_cast. That's a completely orthogonal issue (FWIW, I think that static_cast is theoretically better, but in practice I do not think that it would make any difference with any existing compiler) -- gpd
Emil Dotchevski wrote:
It's just that the standard implementation is to leave it up to the implementer what he wants to do. From 5.2.10 Reinterpret cast section of an October 2008 ballot of the spec: 3 The mapping performed by reinterpret_cast is implementation-defined. [ Note: it might, or might not, produce a representation different from the original value. — end note ] best regards, Patrick
On Thu, Dec 17, 2009 at 5:10 PM, Emil Dotchevski <emildotchevski@gmail.com> wrote:
On Thu, Dec 17, 2009 at 10:07 AM, Giovanni Piero Deretta <gpderetta@gmail.com> wrote:
[snip]
I don't get it why we use reinterpret_cast in the first place. Shouldn't it be static_cast<U*>(static_cast<void*>(x)) ?
Regards, -- Felipe Magno de Almeida
Bo Persson wrote:
But how do we know that the warning is just in case, and that the compiler isn't really confused?
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41874
Or some other compiler that doesn't understand enough to even issue a warning?
Not a problem. The code is correct, the warning isn't.
On Fri, Dec 18, 2009 at 5:20 AM, Peter Dimov <pdimov@pdimov.com> wrote:
Great find.
Alright, then I think the patch with https://svn.boost.org/trac/boost/ticket/3774 should be alright for GCC? -- Dean Michael Berris blog.cplusplus-soup.com | twitter.com/mikhailberis linkedin.com/in/mikhailberis | facebook.com/dean.berris | deanberris.com
On Thu, Dec 17, 2009 at 9:51 AM, Bo Persson <bop@gmb.dk> wrote:
Union is also undefined. You can't portably write one type of data somewhere and then read something else. Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode
Dean Michael Berris wrote:
It's a fair bit dangerous to silence these since it's the compiler's way of telling you that you're invoking undefined behavior, and that when you turn on optimization your code isn't going to work as expected. The following program generates 6 warnings about breaking strict-aliasing rules, and many would dismiss them. The correct output of the program is: 00000020 00200000 but when optimization is turned on it's: 00000020 00000020 THAT's what the warning is trying to tell you, that the optimizer is going to do things that you don't like. In this case seeing that acopy is set to a and never touched again, strict aliasing lets it optimize by just returning the original value of a at the end. uint32_t swaphalves(uint32_t a) { uint32_t acopy=a; uint16_t *ptr=(uint16_t*)&acopy;// can't use static_cast<>, not legal. // you should be warned by that. uint16_t tmp=ptr[0]; ptr[0]=ptr[1]; ptr[1]=tmp; return acopy; } int main() { uint32_t a; a=32; cout << hex << setfill('0') << setw(8) << a << endl; a=swaphalves(a); cout << setw(8) << a << endl; } Here's the (annotated) x86 assembler generated by gcc 4.4.1 for swaphalves. _Z10swaphalvesj: pushl %ebp movl %esp, %ebp subl $16, %esp movl 8(%ebp), %eax # get a in %eax movl %eax, -8(%ebp) # and store in in acopy leal -8(%ebp), %eax # now get eax pointing at acopy (ptr=&acopy) movl %eax, -12(%ebp) # save that ptr at -12(%ebp) movl -12(%ebp), %eax # get the ptr back in %eax movzwl (%eax), %eax # get 16 bits from ptr[0] in eax movw %ax, -2(%ebp) # store the 16 bits into tmp movl -12(%ebp), %eax # get the ptr back in eax addl $2, %eax # bump up by two to get to ptr[1] movzwl (%eax), %edx # get that 16 bits into %edx movl -12(%ebp), %eax # get ptr into eax movw %dx, (%eax) # store the 16 bits into ptr[1] movl -12(%ebp), %eax # get the ptr again leal 2(%eax), %edx # get the address of ptr[1] into edx movzwl -2(%ebp), %eax # get tmp into eax movw %ax, (%edx) # store into ptr[1] movl -8(%ebp), %eax # return original a. leave ret Scary, isn't it? Of course you could use -fno-strict-aliasing to get the right output, but the generated code won't be as good. A better way to accomplish the same thing without the warning's or the incorrect output is to define swaphalves like this: uint32_t swaphalves(uint32_t a) { union swapem{ uint32_t a; uint16_t b[2]; }; swapem s={a}; uint16_t tmp; tmp=s.b[0]; s.b[0]=s.b[1]; s.b[1]=tmp; return s.a; } This follows the rules and helps the compiler generate MUCH better code: _Z10swaphalvesj: pushl %ebp # save the original value of ebp movl %esp, %ebp # point ebp at the stack frame movl 8(%ebp), %eax # get a in eax popl %ebp # get the original ebp value back roll $16, %eax # swap the two halves of a and return it ret Ignore strict aliasing warnings at your peril. But if you really want to -Wno-strict-warnings will do it. Patrick
Patrick Horgan wrote:
THAT's what the warning is trying to tell you, that the optimizer is going to do things that you don't like.
It's trying but not succeeding. The code is fine for two reasons, each of them sufficient. One, it stores an object of type T and then accesses an object of that same type T. Two, accessing an object via a pointer to char/unsigned char is allowed.
...
This follows the rules...
No it doesn't. Writing to one union member and reading another is undefined. You have to use memcpy if you care about the rules.
After reading this exchange, am I right in adding this to the GCC 'what to do about warnings' guidelines section at https://svn.boost.org/trac/boost/wiki/Guidelines/MaintenanceGuidelines? " warning: dereferencing pointer ‘<anonymous>’ does break strict-aliasing rules This warns about undefined behaviour that is likely to cause unwanted results when optimisation is switched on. Recommendation: Fix this by recoding if possible, and document that optimisation may produce wrong results. Include a link to the above example email. Do not suppress the warning with -Wno-strict-warnings - leave that to users after reading the documentation or making their own tests / compile without optimisation.. " Paul --- Paul A. Bristow Prizet Farmhouse Kendal, UK LA8 8AB +44 1539 561830, mobile +44 7714330204 pbristow@hetp.u-net.com
Paul A. Bristow wrote:
It's a lot more complex then that. Sometimes that warning comes from safe type punning such as the placement use in boost optional. I've not convinced myself (yet) that it can't go awry with optimization, in that the optimizer could ignore type-punned access and when you try to retrieve the value get an optimized original value, but others who are quite intelligent and muchly more familiar with it than I believe so. I'm definitely in favor of something like this. I've started working on something myself I wanted to put on that page covering common gcc warnings (mostly drawn from warnings from compiling and using boost), their causes and the fixes, and at 268 lines it's getting tomeish. My post in this conversation came from that document via the magic of cut and paste. Patrick
OK but there is limit to the detail that can/should be provided in Maintenance Guidelines? So should it read "Be very cautious before suppressing the warning".
What I believe we should document is that there *may* be a problem here. And (in the source code too). This might reduce hair loss by users who will assume that something bad could not possibly happen.
I think *you* should edit the wiki gcc section - I'm totally ignorant about gcc.
participants (12)
-
Bo Persson -
Dean Michael Berris -
Emil Dotchevski -
Felipe Magno de Almeida -
Giovanni Piero Deretta -
Mateusz Loskot -
Mathias Gaunard -
Patrick Horgan -
Paul A. Bristow -
Peter Dimov -
Scott McMurray -
Steven Watanabe