Re: [PATCH v1] Travis: also test on 32-bit Linux

From: René Scharfe <l.s.r@web.de>
Date: Mar 11, 2017, 14:17 UTC
Message-ID: <b0d41423-7227-3417-b4b1-fcf47a66f64b@web.de>
In-Reply-To: <xmqqr324wvum.fsf@gitster.mtv.corp.google.com>

Am 11.03.2017 um 00:33 schrieb Junio C Hamano:

Show 10 quoted lines

> René Scharfe <l.s.r@web.de> writes:
> 
>> 	@ depends on r @
>> 	expression E;
>> 	@@
>> 	- *&
>> 	  E
> 
> I guess my source of the confusion is that the tool that understands
> the semantics of the C language still needs to be told about that.

Understanding that E and *&E have the same type does not imply (for a machine) that E alone is preferable.

Show 17 quoted lines

> I was hoping that something that understands C only needs to be told
> only a single rule:
> 
> 	type T
> 	T src, dst
> 
> 	-memcpy(&dst, &src, sizeof(dst));
> 	+dst = src;
> 
> and then can apply that rule to this code in four ways:
> 
> 	struct foo A, *Bp;
> 
> 	memcpy(Bp, &A, sizeof(*Bp));
> 	memcpy(Bp, &A, sizeof(A));
> 	memcpy(&src, dstp, sizeof(A));
> 	memcpy(&src, dstp, sizeof(*Bp));

I guess src is A and dstp is Bp?

Coccinelle does not know that the sizeof expressions are equivalent, but you could normalize them with an additional rule and then use a single memcpy transformation like this:

	@ normalize_sizeof @
	type T;
	T var;
	@@
	- sizeof(var)
	+ sizeof(T)

	@ r @
	type T;
	T *dst;
	T *src;
	@@
	- memcpy(dst, src, sizeof(T));
	+ *dst = *src;

	@ depends on r @
	expression E;
	@@
	- *&
	  E

That would give you what you expected here:

Show 8 quoted lines

> to obtain its rewrite:
> 
> 	struct foo A, *Bp;
> 
> 	*Bp = A;
> 	*Bp = A;
> 	A = *Bp;
> 	A = *Bp;

But that normalization rule would be applied everywhere because it's so broad, and that's probably not what we want. You could replace it with an isomorphism like this one:

	Expression
	@@
	type T;
	T E;
	@@

	sizeof(T) => sizeof E

In your example it allows you to specify sizeof(struct foo) (or a generic sizeof(T) as in rule r above) in the semantic patch and Coccinelle would let that match sizeof(A) and sizeof(*Bp) in the C file as well.

Isomorphisms are kept in a separate file, the default one is /usr/lib/coccinelle/standard.iso on my machine and you can chose a different one with --iso-file. Perhaps we want to have our own for git, but we'd need to synchronize it with upstream from time to time.

There is already a very similar isomorphism rule in the default file, but I don't think I understand it:

	Expression
	@ sizeof_type_expr @
	pure type T; // pure because we drop a metavar
	T E;
	@@

	sizeof(T) => sizeof E

In particular I'm a bit worried about the lack of documentation for "pure", and I don't understand the comment. There's another comment at the top of another rule in the same file:

// pure means that either the whole field reference expression is dropped, // or E is context code and has no attached + code // not really... pure means matches a unitary unplussed metavariable // but this rule doesn't work anyway

Hmm.

Here's an isomorphism that allows you to use "sizeof *src" (the third part for T is not strictly needed for your example):

	Expression
	@ sizeof_equiv @
	type T;
	T E1, E2;
	@@

	sizeof E1 <=> sizeof E2 <=> sizeof(T)

That's the kind of bidirectional equivalence you expected to be part of Coccinelle's standard set of rules, right? With that rule in place the following semantic patch matches your four cases:

	@ r @
	type T;
	T *dst;
	T *src;
	@@
	- memcpy(dst, src, sizeof *dst);
	+ *dst = *src;

	@ depends on r @
	expression E;
	@@
	- *&
	  E

But I'd be careful with that as long as "pure" is present in that standard rule and not fully understood. The isomorphism sizeof_equiv doesn't seem to cause any trouble with our existing semantic patches and the code in master, though.

René