Re: [PATCH] tree-optimization/71831 - __builtin_object_size poor results with no optimization

[Martin Sebor <msebor at gmail dot com>]

On 08/25/2016 01:02 PM, Jeff Law wrote:
> On 08/21/2016 02:29 PM, Martin Sebor wrote:
> > On 08/20/2016 01:26 AM, Jakub Jelinek wrote:
> > > On Fri, Aug 19, 2016 at 04:30:47PM -0600, Martin Sebor wrote:
> > > > The patch looks bigger than it actually is because:
> > > >
> > > > 1) It modifies the return type of the function to bool rather than
> > > >     unsigned HOST_WIDE_INT representing the object size (this was
> > > >     necessary to avoid having its callers misinterpret zero as
> > > >     unknown when it means zero bytes).
> > >
> > > Can you explain why do you need this?  I don't understand why do you
> > > need to
> > > differentiate between unknown and maximum (or minimum for modes 2 and
> > > 3 that
> > > nobody actually uses in real-world), the builtin after all returns the
> > > same
> > > value for both.  If you want to know if the compiler knows the size
> > > precisely, you can request both mode 0 (or 1) and 2 (or 3) and
> > > compare, if
> > > the values are the same, it is the exact size, if there is a range,
> > > then you
> > > have minimum and maximum (and, if minimum is 0, but maximum non-zero,
> > > you
> > > really don't know minimum, if maximum is -1, then you really don't
> > > know the
> > > maximum (no object should be better that big).  For the return value, I
> > > don't see how you could reliably differentiate between the two even
> > > if it
> > > made for whatever strange reason sense - for SSA_NAMEs etc. you have
> > > just
> > > recorded the sizes, not also a flag whether it is unknown or known.
> >
> > The change makes it possible to fold into constants even at -O0 type
> > 2 and 3 calls to the built-in with zero-sized objects.
> This refers to the tests in builtin-object-size-17.c, particularly when
> compiled at -O0, right?

That's right.

> We generally haven't worried much about trying to optimize -O0 under the
> theory that the resulting code should as closely match the source as
> closely as possible -- including leaving dead/unreachable code in the
> resulting .o.  That dead/unreachable code may in fact be useful during a
> debugging session when the developer manually changes the state into a
> "can't happen" state and expects the previously dead/unreachable code to
> execute.

Sure, I understand that.  It just seemed that if the function folded
non-zero sizes it should also fold the zero.  Otherwise the zero size
would not be available to callers like -Wformat-length (at -O0),
causing them to miss potentially important instances of buffer overflow.

> But that's a bit of a stretch in my mind and doesn't really apply in
> this case due to how __b_o_s is expanded.  Ultimately we end up with
> conditionals with compile-time constant arguments, so there's no way for
> the programmer to twiddle things (ok, maybe on a target where the
> compare and branch are separate, the developer could put a break on the
> branch, change the condition codes...  but that's *really* a stretch in
> my mind)
>
> So I don't see any reason to object to this part of the patch --
> essentially it falls under the "don't generate stupid code at -O0" in my
> mind.

Great, thanks.

Martin