Re: [Tinycc-devel] MAJOR bug: tcc doesn't detect duplicate cases in switch statements

[Michael B. Smith]

I am a rare poster here – but I think your patch leads to all sorts of problems.

Please revert it/don’t commit it.

From: tinycc-devel-bounces+michael=theessentialexchange@nongnu.org 
[mailto:tinycc-devel-bounces+michael=theessentialexchange@nongnu.org] On 
Behalf Of Amine Najahi
Sent: Friday, March 11, 2016 12:13 PM
To: tinycc-devel@nongnu.org
Subject: Re: [Tinycc-devel] MAJOR bug: tcc doesn't detect duplicate cases in 
switch statements

Hi Arnold and tcc folks,

Perhaps surprisingly, correcting this bug is quite costly.
Here is a tentative patch. I find it messy but working with dynamic data
and passing the cases to the block function are necessary to handle
an "unlimited number" of cases and nested switch blocks.

Also, since the 'block' function is starting to have too many arguments, I
suggest to create a structure that assembles related arguments such
that def_sym, case_sym, cases, and cases_cnt, and any other...

Does that sound ok?

Regards,
Amine


On Thu, Mar 10, 2016 at 2:41 PM, > 
wrote:
Hi All.

On the latest mob:

$ cat foo.c
int main(int argc, char**argv)
{
int a;

switch (a) {
case 1:
case 2:
break;
case 3:
break;
case 1:
break;
default:
break;
}

return 0;
}
$ PATH=/tmp/tcc/bin:$PATH tcc foo.c
$

$ gcc foo.c -o foo
foo.c: In function ‘main’:
foo.c:11:2: error: duplicate case value
  case 1:
  ^
foo.c:6:2: error: previously used here
  case 1:
  ^

This is pretty serious

Thanks,

Arnold

___
Tinycc-devel mailing list
Tinycc-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/tinycc-devel

___
Tinycc-devel mailing list
Tinycc-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/tinycc-devel

Re: [Tinycc-devel] Alternative tokens for C-application

[Ben Bacarisse]

Michael Matz  writes:

> Ben, have you finished the patch?  If so, please post here, have you
> measured compile time to be unaffected (would be bad if such a seldom
> used feature would cause a slowdown)?

I've not measure but I'd be surprised if there were much impact.
However it's not finished.  All I did was the input side: allowing the
new punctuators to be recognised for what they signify (which is
probably the part people who want them care about), but the output of
tcc -E shows the "canonical" symbols and they "stringify" as those as
well.

I'm not sure of the cost/benefit of finishing it off!

-- 
Ben.

___
Tinycc-devel mailing list
Tinycc-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/tinycc-devel

Re: [Tinycc-devel] Alternative tokens for C-application

[Michael Matz]

Hi,

On Thu, 10 Mar 2016, Мельников Алексей wrote:


--> Urgs.  Are you really using digraphs in any production code?
--> (Yes, tcc tries to support c99, but, well, I mean, digraphs? Seriously? If 
you use EBCDIC you have more serious problems with tcc I guess, and if you don't, 
why use digraphs? Not as bad as trigraphs, but still.)

I am searching new ways to use tcc for web developing. And I think 
digraphs syntax are combined better with html tags.


I see, fair enough.  I'll try to refrain from going "huh?" regarding tcc 
plus web developing (and that <> are special to html and part of every 
digraph), except in this sentence :)


Ben, have you finished the patch?  If so, please post here, have you 
measured compile time to be unaffected (would be bad if such a seldom 
used feature would cause a slowdown)?



Ciao,
Michael.___
Tinycc-devel mailing list
Tinycc-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/tinycc-devel

Re: [Tinycc-devel] tccgen.c: off by one in flexible array members

[Michael Matz]

Hi,

On Fri, 11 Mar 2016, Michael Matz wrote:

This whole thing also points out some deficiencies of tcc to emit error 
messages.  For instance it accepts the initialization


void f(void) {
 struct w q = {"bugs", { 'c' } };
}

(and sets ref->c to 1), even though this is a non-static initialization, 
which is wrong (but the size adjustments needs also to be done for static 
initialization).


And even that is only a GCC extension.  One isn't allowed to initialize 
flex array members, only via malloc and assignments.  (But the GCC 
extension is probably quite prevalent as GCC doesn't even warn about it in 
conformant mode.



Ciao,
Michael.

___
Tinycc-devel mailing list
Tinycc-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/tinycc-devel

Re: [Tinycc-devel] tccgen.c: off by one in flexible array members

[Michael Matz]

Hi,

On Fri, 11 Mar 2016, Michael Matz wrote:


it's now "size += -1 * 4 + 1" (i.e. +=3).


-=3 of course, but you got the idea :)

So, I think it's more correct to special case the ref->c == -1 case only 
(don't adjust size in that case), instead of playing +-1 tricks (as in, 
it's not a off-by-one error).  Will think a bit over dinner :)


After dinner I still agree with me :)  Pushed with a testcase.

This whole thing also points out some deficiencies of tcc to emit error 
messages.  For instance it accepts the initialization


void f(void) {
  struct w q = {"bugs", { 'c' } };
}

(and sets ref->c to 1), even though this is a non-static initialization, 
which is wrong (but the size adjustments needs also to be done for static 
initialization).  Probably this code can be simplified somewhat, but 
that's for somewhen else.



Ciao,
Michael.

___
Tinycc-devel mailing list
Tinycc-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/tinycc-devel

Re: [Tinycc-devel] tccgen.c: off by one in flexible array members

[Michael Matz]

Hi,

On Wed, 9 Mar 2016, Henry Kroll wrote:


I created a + 1 patch that seems to work, but I need to run more tests
before committing.

=
diff --git a/tccgen.c b/tccgen.c
index 3cd28ed..270d11c 100644
--- a/tccgen.c
+++ b/tccgen.c
@@ -5847,7 +5847,7 @@ static void decl_initializer_alloc(CType *type,
AttributeDef *ad, int r,
 tcc_error("unknown type size");
 }
 if (flexible_array)
-size += flexible_array->type.ref->c *
pointed_size(_array->type);
+size += flexible_array->type.ref->c *
pointed_size(_array->type) + 1;
 /* take into account specified alignment if bigger */
 if (ad->a.aligned) {
 if (ad->a.aligned > align)
=

Have a great day!


This still isn't quite right.  The thing is, type->ref.c is -1 for an 
unused flex array, so with your example ("char mem[]") your change 
basically boils down to "size += -1 * 1 + 1" (i.e. +=0).  But with a 
different type (say int) it's now "size += -1 * 4 + 1" (i.e. +=3).  So 
this example still fails the same way:


-
#include 
struct w {
char *data; int mem[];
};
int main (void) {
   char s[9]="nonono"; struct w q = {"bugs"};
   printf ("tcc has %s %s\n", s, q.data);
   return !s[0];
}
-

(only changed the mem[] member type).  The +1 also looks conceptually 
wrong, we try to adjust the size by some number of elements.  The example 
works of course if you'd do

  size += (flexible_array->type.ref->c + 1)
  * pointed_size(_array->type)

But then it'll break this example: (well, not break it, but it seems it'd 
allocate more then necessary), which actually does use the flex member:


-
#include 
struct w {
char *data; int mem[];
};
int main (void) {
   char s[9]="nonono";
   struct w q = {"bugs", { 1 }};
   printf ("tcc has %s %s\n", s, q.data);
   return !s[0];
}
-

Here type->ref.c (after parsing the initializer) will be 1, the number of 
elements used, and the original size calculation seems correct. 
type->ref.c will be 0 if the flex member is there but empty (in the 
initializer), so for that case the original is also correct.


So, I think it's more correct to special case the ref->c == -1 case only 
(don't adjust size in that case), instead of playing +-1 tricks (as in, 
it's not a off-by-one error).  Will think a bit over dinner :)



diff --git a/tccgen.c b/tccgen.c
index 270d11c..896a520 100644
--- a/tccgen.c
+++ b/tccgen.c
@@ -5846,8 +5846,10 @@ static void decl_initializer_alloc(CType *type, 
AttributeDef *ad, int r,
 if (size < 0)
 tcc_error("unknown type size");
 }
-if (flexible_array)
-size += flexible_array->type.ref->c * 
pointed_size(_array->type) + 1;
+if (flexible_array
+   /* If the flex member was used in the initializer.  */
+   && flexible_array->type.ref->c > 0)
+size += flexible_array->type.ref->c * 
pointed_size(_array->type);
 /* take into account specified alignment if bigger */
 if (ad->a.aligned) {
 if (ad->a.aligned > align)


Ciao,
Michael.___
Tinycc-devel mailing list
Tinycc-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/tinycc-devel

Re: [Tinycc-devel] MAJOR bug: tcc doesn't detect duplicate cases in switch statements

[Michael Matz]

Hi,

On Fri, 11 Mar 2016, Amine Najahi wrote:

> Perhaps surprisingly, correcting this bug is quite costly. Here is a 
> tentative patch. I find it messy but working with dynamic data and 
> passing the cases to the block function are necessary to handle an 
> "unlimited number" of cases and nested switch blocks.
> 
> Also, since the 'block' function is starting to have too many arguments, 
> I suggest to create a structure that assembles related arguments such 
> that def_sym, case_sym, cases, and cases_cnt, and any other...
> 
> Does that sound ok?

I'm not a big fan of this solution.  First, the bug can't be as MAJOR as 
the subject wants to make us believe.  After all it wasn't noticed since 
the last $MANY years.  Secondly it's inherently costly (as you notice) to 
do such checking in a single-pass compiler; that's simply expected of the 
approach that TCC takes to reach the speed.  Third, you don't handle the 
GNU extension "case 1 .. 3" at all.  I think you handle this correctly:

switch (a) {
  case 1:
switch (b) {
  case 1: break; // no error here
}
break;
  case 1: break;  // but an error here
}

so, that's nice.  But still, I don't think it's worth it.  (Hey, in a way 
not erroring is even sort of logical, the error is only there because the 
standard requires it; but if looking at the mirrored situation in an 
if-cascade, "if (a == 1); else if (a == 1);" there's no error required).

I think there are quite many required diagnostics we don't do because of 
the single-pass nature and I don't see why this one only should be fixed.

Perhaps I could be convinced by some speed measurements on a relevant 
source base (not only tcc itself).


Ciao,
Michael.
P.S: Oh, and the dynarray API and usage is super ugly ;-)

___
Tinycc-devel mailing list
Tinycc-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/tinycc-devel

Re: [Tinycc-devel] MAJOR bug: tcc doesn't detect duplicate cases in switch statements

[Amine Najahi]

Hi Arnold and tcc folks,

Perhaps surprisingly, correcting this bug is quite costly.
Here is a tentative patch. I find it messy but working with dynamic data
and passing the cases to the block function are necessary to handle
an "unlimited number" of cases and nested switch blocks.

Also, since the 'block' function is starting to have too many arguments, I
suggest to create a structure that assembles related arguments such
that def_sym, case_sym, cases, and cases_cnt, and any other...

Does that sound ok?

Regards,
Amine


On Thu, Mar 10, 2016 at 2:41 PM,  wrote:

> Hi All.
>
> On the latest mob:
>
> $ cat foo.c
> int main(int argc, char**argv)
> {
> int a;
>
> switch (a) {
> case 1:
> case 2:
> break;
> case 3:
> break;
> case 1:
> break;
> default:
> break;
> }
>
> return 0;
> }
> $ PATH=/tmp/tcc/bin:$PATH tcc foo.c
> $
>
> $ gcc foo.c -o foo
> foo.c: In function ‘main’:
> foo.c:11:2: error: duplicate case value
>   case 1:
>   ^
> foo.c:6:2: error: previously used here
>   case 1:
>   ^
>
> This is pretty serious
>
> Thanks,
>
> Arnold
>
> ___
> Tinycc-devel mailing list
> Tinycc-devel@nongnu.org
> https://lists.nongnu.org/mailman/listinfo/tinycc-devel
>
From 70364d9a8d1c57da490e0187421ae94af42bf579 Mon Sep 17 00:00:00 2001
From: Amine Najahi 
Date: Fri, 11 Mar 2016 17:54:11 +0100
Subject: [PATCH] Fix duplicate case values bug.

---
 tccgen.c | 36 +++-
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/tccgen.c b/tccgen.c
index 3cd28ed..ef4b2e6 100644
--- a/tccgen.c
+++ b/tccgen.c
@@ -80,7 +80,7 @@ static int parse_btype(CType *type, AttributeDef *ad);
 static void type_decl(CType *type, AttributeDef *ad, int *v, int td);
 static void parse_expr_type(CType *type);
 static void decl_initializer(CType *type, Section *sec, unsigned long c, int first, int size_only);
-static void block(int *bsym, int *csym, int *case_sym, int *def_sym, int case_reg, int is_expr);
+static void block(int *bsym, int *csym, int *case_sym, int *def_sym, int case_reg, int is_expr, void*** cases, int* cases_cnt);
 static void decl_initializer_alloc(CType *type, AttributeDef *ad, int r, int has_init, int v, int scope);
 static int decl0(int l, int is_for_loop_init);
 static void expr_eq(void);
@@ -3848,7 +3848,7 @@ ST_FUNC void unary(void)
 save_regs(0); 
 /* statement expression : we do not accept break/continue
inside as GCC does */
-block(NULL, NULL, NULL, NULL, 0, 1);
+block(NULL, NULL, NULL, NULL, 0, 1, NULL, 0);
 skip(')');
 } else {
 gexpr();
@@ -4816,7 +4816,7 @@ static void label_or_decl(int l)
 }
 
 static void block(int *bsym, int *csym, int *case_sym, int *def_sym, 
-  int case_reg, int is_expr)
+  int case_reg, int is_expr, void*** cases, int* cases_cnt)
 {
 int a, b, c, d;
 Sym *s, *frame_bottom;
@@ -4842,13 +4842,13 @@ static void block(int *bsym, int *csym, int *case_sym, int *def_sym,
 gexpr();
 skip(')');
 a = gvtst(1, 0);
-block(bsym, csym, case_sym, def_sym, case_reg, 0);
+block(bsym, csym, case_sym, def_sym, case_reg, 0, cases, cases_cnt);
 c = tok;
 if (c == TOK_ELSE) {
 next();
 d = gjmp(0);
 gsym(a);
-block(bsym, csym, case_sym, def_sym, case_reg, 0);
+block(bsym, csym, case_sym, def_sym, case_reg, 0, cases, cases_cnt);
 gsym(d); /* patch else jmp */
 } else
 gsym(a);
@@ -4861,7 +4861,7 @@ static void block(int *bsym, int *csym, int *case_sym, int *def_sym,
 skip(')');
 a = gvtst(1, 0);
 b = 0;
-block(, , case_sym, def_sym, case_reg, 0);
+block(, , case_sym, def_sym, case_reg, 0, cases, cases_cnt);
 gjmp_addr(d);
 gsym(a);
 gsym_addr(b, d);
@@ -4898,7 +4898,7 @@ static void block(int *bsym, int *csym, int *case_sym, int *def_sym,
 if (tok != '}') {
 if (is_expr)
 vpop();
-block(bsym, csym, case_sym, def_sym, case_reg, is_expr);
+block(bsym, csym, case_sym, def_sym, case_reg, is_expr, cases, cases_cnt);
 }
 }
 /* pop locally defined labels */
@@ -5053,7 +5053,7 @@ static void block(int *bsym, int *csym, int *case_sym, int *def_sym,
 gsym(e);
 }
 skip(')');
-block(, , case_sym, def_sym, case_reg, 0);
+block(, , case_sym, def_sym, case_reg, 0, cases, cases_cnt);
 gjmp_addr(c);
 gsym(a);
 gsym_addr(b, c);
@@ -5066,7 +5066,7 @@ static void block(int *bsym, int *csym, int *case_sym, int *def_sym,
 b = 0;
 d = ind;