| Message ID | 4223944c-1f2e-bfdf-9895-298df2373ada@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Dec 01, 2016 at 11:43:19PM -0700, Jeff Law wrote: > > Martin's alloca work flagged this code as problematical. Essentially if we > had a statement with no operands and the statement was not in the hash > table, then we could end up performing alloca (0), which is inadvisable. I still don't understand why it is inadvisable. alloca(0) is not undefined behavior. It can return NULL, or non-unique pointer, or a unique pointer, and/or cause freeing of already left alloca blocks (like any other alloca call). None of that is a problem here. If num is 0, then copy is just set and never used. I expect most if not all gcc uses of alloca where the count can be 0 are like that. Jakub
On 12/02/2016 10:58 AM, Jakub Jelinek wrote: > On Thu, Dec 01, 2016 at 11:43:19PM -0700, Jeff Law wrote: >> >> Martin's alloca work flagged this code as problematical. Essentially if we >> had a statement with no operands and the statement was not in the hash >> table, then we could end up performing alloca (0), which is inadvisable. > > I still don't understand why it is inadvisable. alloca(0) is not undefined > behavior. It can return NULL, or non-unique pointer, or a unique pointer, > and/or cause freeing of already left alloca blocks (like any other alloca > call). > None of that is a problem here. If num is 0, then copy is just set and > never used. > I expect most if not all gcc uses of alloca where the count can be 0 are > like that. It won't cause any problems in this and probably most instances, but leaving the code in its prior state is simply wrong from a maintenance standpoint. I'd much rather have the code explicitly and safely handle the zero operands case so that if someone makes a change later they don't have to worry about whether or not they're accessing memory which was never allocated. Additionally, it removes a false positive from the warning, thus making less noise. It's not unlike the strictly unnecessary initializations we do to shut up -Wuninitialized. Jeff
On Fri, Dec 02, 2016 at 11:02:33AM -0700, Jeff Law wrote: > It won't cause any problems in this and probably most instances, but leaving > the code in its prior state is simply wrong from a maintenance standpoint. > > I'd much rather have the code explicitly and safely handle the zero operands > case so that if someone makes a change later they don't have to worry about > whether or not they're accessing memory which was never allocated. > > Additionally, it removes a false positive from the warning, thus making less > noise. > > It's not unlike the strictly unnecessary initializations we do to shut up > -Wuninitialized. But -Wuninitialized also found tons of real-world bugs. Do we have a single real-world example where such a warning would actually be useful (as in, there would be an actual bug)? Otherwise we are adding workarounds for a warning that just forces people to add those workarounds, but doesn't improve code in the wild in any way. Jakub
On 12/02/2016 11:11 AM, Jakub Jelinek wrote: > On Fri, Dec 02, 2016 at 11:02:33AM -0700, Jeff Law wrote: >> It won't cause any problems in this and probably most instances, but leaving >> the code in its prior state is simply wrong from a maintenance standpoint. >> >> I'd much rather have the code explicitly and safely handle the zero operands >> case so that if someone makes a change later they don't have to worry about >> whether or not they're accessing memory which was never allocated. >> >> Additionally, it removes a false positive from the warning, thus making less >> noise. >> >> It's not unlike the strictly unnecessary initializations we do to shut up >> -Wuninitialized. > > But -Wuninitialized also found tons of real-world bugs. Do we have a single > real-world example where such a warning would actually be useful (as in, > there would be an actual bug)? Otherwise we are adding workarounds for a > warning that just forces people to add those workarounds, but doesn't > improve code in the wild in any way. Have you looked in depth at the lto.c issue it flagged? I can't prove that one is safe. And more generally, an under-sized allocation is a security risk. We should continue to try and identify those and warn for them. jeff
On Fri, Dec 02, 2016 at 11:13:29AM -0700, Jeff Law wrote: > >But -Wuninitialized also found tons of real-world bugs. Do we have a single > >real-world example where such a warning would actually be useful (as in, > >there would be an actual bug)? Otherwise we are adding workarounds for a > >warning that just forces people to add those workarounds, but doesn't > >improve code in the wild in any way. > Have you looked in depth at the lto.c issue it flagged? I can't prove that > one is safe. If you mean the tree *map = XALLOCAVEC (tree, 2 * len); call, then I strongly doubt it can actually ever be called with len == 0. There is tree_scc *scc = (tree_scc *) alloca (sizeof (tree_scc) + (len - 1) * sizeof (tree)); a few lines above this and len is unsigned int, therefore for len 0 this will try to (on 64-bit host) alloca (32 + 0xffffffff * 4UL), i.e. alloca (0x40000001cUL) and I'm pretty sure on most hosts alloca of 16GB won't really work. Even if this succeeded for whatever reason, what problem do you see with map = alloca (0)? > And more generally, an under-sized allocation is a security risk. We should Sure. But I really don't see 0 as being special in any way. We do support zero sized arrays and IMNSHO we want to support 0 sized alloca, it is very much the same thing. We also do support VLAs with 0 size. Jakub
diff --git a/gcc/ChangeLog b/gcc/ChangeLog index c3170c0..75881ee 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,9 @@ +2016-12-01 Jeff Law <law@redhat.com> + + * tree-ssa-threadedge.c + (record_temporary_equivalences_from_stmts_at_dest): Avoid temporary + propagation of operands if there are no operands. + 2016-12-02 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/78586 diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c index 534292c..3fdd59e 100644 --- a/gcc/tree-ssa-threadedge.c +++ b/gcc/tree-ssa-threadedge.c @@ -328,9 +328,10 @@ record_temporary_equivalences_from_stmts_at_dest (edge e, SSA_NAME_VALUE in addition to its own lattice. */ cached_lhs = gimple_fold_stmt_to_constant_1 (stmt, threadedge_valueize); - if (!cached_lhs - || (TREE_CODE (cached_lhs) != SSA_NAME - && !is_gimple_min_invariant (cached_lhs))) + if (NUM_SSA_OPERANDS (stmt, SSA_OP_ALL_USES) != 0 + && (!cached_lhs + || (TREE_CODE (cached_lhs) != SSA_NAME + && !is_gimple_min_invariant (cached_lhs)))) { /* We're going to temporarily copy propagate the operands and see if that allows us to simplify this statement. */
Martin's alloca work flagged this code as problematical. Essentially if we had a statement with no operands and the statement was not in the hash table, then we could end up performing alloca (0), which is inadvisable. We can safely just avoid the whole block of code if there are no operands. Bootstrapped and regression tested on x86_64-linux-gnu. Installed on the trunk. Jeff commit 790bfbe1974f0b8d1cb23f73635221f4ccb4dafe Author: law <law@138bc75d-0d04-0410-961f-82ee72b054a4> Date: Fri Dec 2 06:40:57 2016 +0000 * tree-ssa-threadedge.c (record_temporary_equivalences_from_stmts_at_dest): Avoid temporary propagation of operands if there are no operands. git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@243152 138bc75d-0d04-0410-961f-82ee72b054a4