Re: [Tinycc-devel] missing check after calling type_size in classify_x86_64_arg

[Michael Matz]

Hi,

On Sun, 23 Jun 2019, Pascal Cuoq wrote:

> > The patch definitely goes into the right direction, though it seem more verbose 
> > than necessary.  I'd just test for functions or incomplete types (via 
> > type_size), and then you have the opportunity to retain the more precise error 
> > message for the former, ala
> >
> >  if (func)
> >    tcc_error ...
> >  else if (type_size < 0)
> >    tcc_error ...
> >  okay ...
>
> I considered this but:
> - the single generic error message seemed consistent with a compiler which 
> advertises its small size and its speed, and which was accepting these programs 
> until recently,
> - but even producing a generic error message, functions would have to remain as 
> a separate case because type_size returns 1 for them,
> - and also type_size returns 1 for void, so that would have to be another 
> separate case if we wish to reject arrays of void.

Sure, but to avoid two cases you added five, and encoded the knowledge 
about the ->c field in still another place.  What I meant is adding two 
lines like so:

diff --git a/tccgen.c b/tccgen.c
index c65cf76..84e0953 100644
--- a/tccgen.c
+++ b/tccgen.c
@@ -4507,6 +4507,8 @@ static int post_type(CType *type, AttributeDef *ad, int 
storage, int td)
         post_type(type, ad, storage, 0);
         if (type->t == VT_FUNC)
             tcc_error("declaration of an array of functions");
+        else if ((type->t & VT_BTYPE) == VT_VOID || type_size(type, &l) < 0)
+            tcc_error("wrong!");
         t1 |= type->t & VT_VLA;

         if (t1 & VT_VLA) {

> Two separate cases mean we have to compute type->t & VT_BTYPE. If we 
> call type_size it will recompute it, and store an alignment that we do 
> not require to a variable that we will have to declare. It doesn't look 
> like it's shorter or faster.

It's definitely shorter, see above :)  If you consider 
compilation speed you'd have to compare with an optimizing compiler 
anyway, and then none of the above unoptimality matters.  Even without an 
optimizing compiler (say, with TCC itself) the code generated for the 
switch will be larger than the above.

Thing is: we have the type_size helper routine, so it should be used.

In the end of course some of this is merely a question about style and 
that's subjective.  Personally I like things being done in fewer lines of 
code (without breaking the formating style) first, and then 
optimize hot spots.  This place definitely isn't one.

> If you think that this much code should be put in a function, that might 
> be useful elsewhere, I can do that,

Only if multiple users are added; the block (even your version) isn't 
complex enough to warrant a separate function with just a single caller 
IMHO.


Ciao,
Michael.