kicad-developers team mailing list archive

[Chris Pavlina <pavlina.chris@xxxxxxxxx>]

Declaring the "unsigned long long" volatile should be (and is, I tested)
sufficient to force the compiler to actually read that integer at
runtime and not trust that it came from a cast pointer.

"unsigned long long" is horribly ugly though - surely we have uintptr_t
in cstdint on all our supported platforms? It's _meant_ for this, though
technically optional.

Also, if we were to do this, I'd be strongly in favor of factoring it
out into a function with some explanation as to why. Something like:

/* Some very bad people have written code to test whether a reference is
 * NULL. As permitted by standard, the compiler optimizes this out to
 * punish these very bad people. This function forces it to perform the
 * test, as an alternative to rewriting the horrors contained below.
 */

template<typename T>
bool ref_is_null( T& x )
{
    volatile uintptr_t px = (uintptr_t) &x;
    return px == 0;
}

On Thu, Sep 29, 2016 at 10:14:47AM +1000, Cirilo Bernardo wrote:
> Just in case NULL is indeed deliberately cast to a ref (as in the case of
> OpenCascade), how about  replacing this test with something like:
> 
> void foo(int &i)
> {
>     unsigned long long z = (unsigned long long) &i;
> 
>     if( z < 2 )
>         std::cout << "blah";
> }
> 
> Of course if the compiler is *too clever* it will realize that "2" is
> still an invalid
> pointer on 32 and 64-bit addressing schemes and still optimize this out, but
> my gcc 6.2.0 isn't that clever. The only *other* problem is that
> unsigned long long
> is to be at least 64b but I have no idea if there is anything in the language
> specification to guarantee it is at least as large as a pointer type.
> 
> - Cirilo
> 
> 
> 
> On Thu, Sep 29, 2016 at 1:57 AM, Wayne Stambaugh <stambaughw@xxxxxxxxx> wrote:
> > I didn't test what the compiler generated but I suspected that was the
> > case from warning message. If that's the case then I will apply the
> > patch but it does mask the underlying issue which could fail spectacularly.
> >
> > On 9/28/2016 11:43 AM, Chris Pavlina wrote:
> >> Well, as for removing them being a good or bad idea - the compiler is
> >> probably doing that anyway. I just tested on GCC, at -O1 or greater it
> >> just assumes all references are non-NULL and replaces any "&ref != 0"
> >> with a hard-coded boolean 'true'. So... there's a pretty good chance we
> >> haven't had those checks for quite some time.
> >>
> >> On Wed, Sep 28, 2016 at 09:24:40AM -0400, Wayne Stambaugh wrote:
> >>> I normally don't like to rant but OMFG!  I just looked at the code path
> >>> that lead to this wonderful piece of code and I'm now regretting that.
> >>> You just cannot unsee that.  See if you can follow the logic (assuming
> >>> logic was actually used to write this code) backwards from
> >>> Triangle::NeighborAcross().  I'm not sure removing the invalid null
> >>> reference checks is a good idea even though I've seen the compiler
> >>> complain about it and agree with it.  In what universe do you blindly
> >>> convert a potentially NULL pointer into a C++ reference, pass it to the
> >>> caller, and go merrily on you way?  This really needs to be rewritten
> >>> using pointers with the appropriate NULL pointer checks along the way.
> >>> It's hard to imagine this ever worked properly.
> >>>
> >>> On 9/27/2016 1:33 PM, Simon Richter wrote:
> >>>>
> >>>> Triangle::NeighborAcross returns a reference, which must refer to a valid
> >>>> object whose address cannot be 0. Thus, this test is nonsensical.
> >>>> ---
> >>>>  polygon/poly2tri/sweep/sweep.cc | 16 ----------------
> >>>>  1 file changed, 16 deletions(-)
> >>>>
> >>>>
> >>>>
> >>>> _______________________________________________
> >>>> Mailing list: https://launchpad.net/~kicad-developers
> >>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> >>>> Unsubscribe : https://launchpad.net/~kicad-developers
> >>>> More help   : https://help.launchpad.net/ListHelp
> >>>>
> >>>
> >>> _______________________________________________
> >>> Mailing list: https://launchpad.net/~kicad-developers
> >>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> >>> Unsubscribe : https://launchpad.net/~kicad-developers
> >>> More help   : https://help.launchpad.net/ListHelp
> >
> > _______________________________________________
> > Mailing list: https://launchpad.net/~kicad-developers
> > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> > Unsubscribe : https://launchpad.net/~kicad-developers
> > More help   : https://help.launchpad.net/ListHelp