On 12/01/2017 11:06 AM, Martin Sebor wrote: > On 12/01/2017 01:26 AM, Jeff Law wrote: >> On 11/30/2017 01:30 PM, Martin Sebor wrote: >>> On 11/22/2017 05:03 PM, Jeff Law wrote: >>>> On 11/21/2017 12:07 PM, Martin Sebor wrote: >>>>> On 11/21/2017 09:55 AM, Jeff Law wrote: >>>>>> On 11/19/2017 04:28 PM, Martin Sebor wrote: >>>>>>> On 11/18/2017 12:53 AM, Jeff Law wrote: >>>>>>>> On 11/17/2017 12:36 PM, Martin Sebor wrote: [ Lots of snipping ] >> >> In fact, if I look at how we handle expand_builtin_mempcpy we have: >> >> /* Avoid expanding mempcpy into memcpy when the call is determined >> to overflow the buffer. This also prevents the same overflow >> from being diagnosed again when expanding memcpy. */ >> if (!check_memop_sizes (exp, dest, src, len)) >> return NULL_RTX; >> >> While that's not strictly meant to be an optimization, it is in effect >> changing the code we generate based on the return value of >> check_memop_sizes, which comes from compute_objsize. Thankfully, the >> worst that happens here is we'll fail to turn the mempcpy into a memcpy >> if compute_objsize/check_memop_sizes returns a value that is not >> strictly correct. But I think it highlights how easy it is to end up >> having code generation changing based on the results of compute_objsize. > > I think you have misread the code (which is easy to do), so I'm > not sure it does highlight it. Just to be clear, I'm convinced the code should DTRT as-written and that it's behavior is not changed by your patch.
My point is that these routines should not generally be used to influence code generation/optimization decisions. In an ideal world we could express and enforce that rule. But we don't live in that ideal world and as a result it's relatively easy to use those routines to influence code generation decisions without even knowing it. So again, anytime we have something like what we see above where we call check_memop_sizes/compute_objsize and the result is used to select between paths that change code generation/optimization we need a comment. We also need a comment in compute_objsize to discourage its use in any contexts where the result affects code generation/optimization. Again, the code is safe as-is and not affected by your patch. I'm just asking for a comment for those functions and at any site where those functions are used to influence code generation/optimization decisions. [ More snipping.. Hopefully not losing too much context.. ] >> SO would you rather go with baking the inexact nature of the return >> value into the API or the pair of comments noted above? > > I added the comment to compute_objsize in the last iteration > of the patch. > > As I explained above, expand_builtin_mempcpy isn't affected by > this patch. The comments above and within in the function > already explain what what happens when a buffer overflow is > detected and why so I'm not sure what else to say there. If > I misunderstood your suggestion please clarify. The comment I'm looking for in expand_builtin_mempcpy would be something like this: /* Policy does not generally allow using compute_objsize (which is used internally by check_memop_size) to change code generation or drive optimization decisions. In this instance it is safe because the code we generate has the same semantics regardless of the return value of check_memop_sizes. Exactly the same amount of data is copied and the return value is exactly the same in both cases. Furthermore, check_memop_size always uses mode 0 for the call to compute_objsize, so the imprecise nature of compute_objsize is avoided. */ > > The other change you are suggesting means restoring the false > negatives in Object Size modes 2 and 3 where my patch detected > true positives. Here's an example to demonstrate the effect. > With my original patch, the overflow in both functions below > is diagnosed with all -Wstringop-overflow=N arguments. With > the change you asked for, only the memcpy overflow is diagnosed > with all arguments. The strncpy overflow is only diagnosed > with N=1 and 2, and suppressed with N=3 and N=4. That's clearly > a worse outcome, but in the interest of moving forward I've made > the change. I think the overall improvement is worthwhile despite > this flaw. So given the comments about restrictions in how compute_objsize is used, I don't think we need to make the change I originally asked for. Go with whatever you think is best WRT handling of that bound computation. Either computation of that bound has a degree of imprecision due to range involvement and makes the result unsuitable for influencing code generation or optimization. In the compute_objsize comment I'd use "The function is intended for diagnostics and should not be used to influence code generation or optimization." I think that's slightly better. OK with either computation the updated comment for compute_objsize. Please add a comment to the mempcpy bits as well. Jeff
