kicad-developers team mailing list archive

[Chris Pavlina <pavlina.chris@xxxxxxxxx>]

The whole thing gives me shivers. I'd much rather rewrite the code to
not do this at all, but if we're going to try to "fix" it "in place",
I'd much rather keep the comparison in one documented location than
scatter them around. It will be quite unobvious later why we were
dancing around casting things in weird ways otherwise.

I won't have time to commit it myself for a little while, though.

On Wed, Sep 28, 2016 at 08:55:41PM -0400, Wayne Stambaugh wrote:
> Ewww!  Gives me shivers but it drives the point home until it can be
> fixed properly.  I'm fine with you committing this until then.
> 
> On 9/28/2016 8:31 PM, Chris Pavlina wrote:
> > 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
>