linux-sparse.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Phil Carmody <phil@dovecot.fi>
To: Josh Triplett <josh@joshtriplett.org>
Cc: Phil Carmody <phil@dovecot.fi>,
	sparse@chrisli.org, linux-sparse@vger.kernel.org
Subject: Re: [PATCH 2/3] sparse: detect non-sign-extended masks created by '~'
Date: Mon, 9 Jun 2014 19:05:41 +0300	[thread overview]
Message-ID: <20140609160529.GA14166@phil.dovecot.net> (raw)
In-Reply-To: <20140609133424.GB11986@thin>

On Mon, Jun 09, 2014 at 06:34:24AM -0700, Josh Triplett wrote:

Thanks for the  quick response.

> On Mon, Jun 09, 2014 at 02:58:01PM +0300, Phil Carmody wrote:
> > Consider the operation of rounding up to the nearest multiple of a power of 2.
> > e.g.  #define ALLOC_SIZE(t) ((sizeof(t) + ASIZE - 1) & ~(ASIZE - 1))
> > 
> > If ASIZE is unfortunately defined as an unsigned type smaller than size_t,
> > then the ~ will not undergo sign-bit extension, and the incorrect mask will
> > be used. If used in a memory allocation context this could be fatal.
> > 
> > Warn about such dubious 'large op ~short' usage.
> > 
> > Signed-off-by: Phil Carmody <phil@dovecot.fi>
> > ---
> >  evaluate.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/evaluate.c b/evaluate.c
> > index 9052962..c0f3c91 100644
> > --- a/evaluate.c
> > +++ b/evaluate.c
> > @@ -189,6 +189,14 @@ left:
> >  	return left;
> >  }
> >  
> > +static int is_bigger_int_type(struct symbol *left, struct symbol *right)
> > +{
> > +	left = integer_promotion(left);
> > +	right = integer_promotion(right);
> > +
> > +	return (left->bit_size > right->bit_size);
> > +}
> > +
> >  static int same_cast_type(struct symbol *orig, struct symbol *new)
> >  {
> >  	return orig->bit_size == new->bit_size &&
> > @@ -927,6 +935,19 @@ static struct symbol *evaluate_binop(struct expression *expr)
> >  					op,
> >  					right_not ? "!" : "");
> >  
> > +			left_not  = expr->left->type == EXPR_PREOP
> > +					&& expr->left->op == '~';
> > +			right_not = expr->right->type == EXPR_PREOP
> > +			                && expr->right->op == '~';
> 
> Ah, now I see why you wanted these to not use "const".  Fair enough.
> "bool" still seems like the right type, though.

There did seem to be general bool-avoidance in the code, it would have been
my preference too.
 
> > +			if ((left_not && is_bigger_int_type(rtype, ltype)
> > +			     && (ltype->ctype.modifiers & MOD_UNSIGNED)) ||
> > +			    (right_not && is_bigger_int_type(ltype, rtype)
> > +			     && (rtype->ctype.modifiers & MOD_UNSIGNED)))
> 
> You might consider wrapping the common expression here, along with the
> corresponding previous _not expression, into a function, and then
> calling it twice, flipping the arguments around for the second call.

Yes, that makes sense.
 
> > +				warning(expr->pos, "dubious: %sx %c %sy",
> > +				        left_not ? "~" : "",
> > +					op,
> > +					right_not ? "~" : "");
> 
> What happens here if left_not && right_not?  Should this warning still
> occur?  I *think* it still makes sense for it to, but the warning
> message might prove less informative.

You're right, the message wouldn't identify which was the operand that
was not being sign extended. I can pull the warning itself into the helper
function I create for the test.

> > +
> >  			ltype = usual_conversions(op, expr->left, expr->right,
> >  						  lclass, rclass, ltype, rtype);
> >  			ctype = rtype = ltype;

Thanks for your comments. A v2 will be forthcoming...

Cheers,
Phil

  reply	other threads:[~2014-06-09 16:10 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-06-09 11:57 [PATCH 0/3] catch non-sign-extended '~' brainos Phil Carmody
2014-06-09 11:58 ` [PATCH 1/3] sparse: Just use simple ints for decision variables Phil Carmody
2014-06-09 11:58   ` [PATCH 2/3] sparse: detect non-sign-extended masks created by '~' Phil Carmody
2014-06-09 11:58     ` [PATCH 3/3] validation: dubious bitwise operations with nots Phil Carmody
2014-06-09 13:36       ` Josh Triplett
2014-06-09 13:34     ` [PATCH 2/3] sparse: detect non-sign-extended masks created by '~' Josh Triplett
2014-06-09 16:05       ` Phil Carmody [this message]
2014-06-09 13:27   ` [PATCH 1/3] sparse: Just use simple ints for decision variables Josh Triplett
2014-06-10  7:54 ` [PATCHv2 0/3] catch non-sign-extended '~' brainos Phil Carmody
2014-06-10  7:54   ` [PATCHv2 1/3] sparse: Just use simple ints for decision variables Phil Carmody
2014-06-10  7:54     ` [PATCHv2 2/3] sparse: detect non-sign-extended masks created by '~' Phil Carmody
2014-06-10  7:54       ` [PATCHv2 3/3] validation: dubious bitwise operations with bitwise nots Phil Carmody
2014-06-27 11:19   ` [PATCHv2 0/3] catch non-sign-extended '~' brainos Phil Carmody
2014-06-27 17:16     ` Christopher Li
2014-06-30  8:56       ` Phil Carmody
     [not found]         ` <CANeU7Q=Z=Xac_T3JRAyqo_fF4LAKD-MM41NYz+nDstDutcVUfA@mail.gmail.com>
2014-06-30 17:27           ` Christopher Li
2014-07-01 11:30           ` Phil Carmody
2014-07-01 19:42             ` Christopher Li
2014-07-02  7:43               ` Phil Carmody
2014-07-02  8:51                 ` Christopher Li
2014-07-02  9:28                   ` Phil Carmody

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140609160529.GA14166@phil.dovecot.net \
    --to=phil@dovecot.fi \
    --cc=josh@joshtriplett.org \
    --cc=linux-sparse@vger.kernel.org \
    --cc=sparse@chrisli.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).