Bug 964307 - Apparent miscompilation of scilab code on i686 with 4.8
Summary: Apparent miscompilation of scilab code on i686 with 4.8
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: gcc
Version: 19
Hardware: i686
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Jakub Jelinek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 914505
TreeView+ depends on / blocked
 
Reported: 2013-05-17 19:48 UTC by Orion Poplawski
Modified: 2013-05-17 21:00 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-05-17 21:00:36 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
Pre-processed source for sci_strindex.c (238.59 KB, text/plain)
2013-05-17 19:48 UTC, Orion Poplawski
no flags Details

Description Orion Poplawski 2013-05-17 19:48:22 UTC
Created attachment 749458 [details]
Pre-processed source for sci_strindex.c

Description of problem:


I'm trying to track down a very strange error with the 32-bit version of scilab compiled with gcc 4.8.  Versions compiled with 4.7.2 appear to be fine.

The simplest demonstration with scilab code is:

scilab -ns -nwni -e "disp(strindex('/builddir/build/BUILD/scilab-5.4.1/modules/graphics/macros/*.sci','/'));quit;"

    1.    10.    0.    0.    0.    0.    0.    0.

This should return a list of the index positions of "/" in the string, but only the first two are returned.

strindex is implemented by sci_strindex in scilab-5.4.1/modules/string/sci_gateway/c/sci_strindex.c.  At the end of the function it copies its working data (in "values") into some kind of structure that appears to be return variable in scilab.  When I walk through this with gdb I see:


(gdb) 
289             for ( i = 0 ; i < nbValues ; i++ )
(gdb) print nbValues
$32 = 8
(gdb) n
291                 stk(outIndex)[i] = (double)values[i].data ;
(gdb) 
289             for ( i = 0 ; i < nbValues ; i++ )
(gdb) n
291                 stk(outIndex)[i] = (double)values[i].data ;
(gdb) 
289             for ( i = 0 ; i < nbValues ; i++ )
(gdb) n
291                 stk(outIndex)[i] = (double)values[i].data ;
1: (double)values[i].data = 10
(gdb) 
293             LhsVar(1) = Rhs + 1 ;

It appears that loop is only executed 2 or 3 times.

This first appeared with the scilab build on 2013-02-18.

I have no idea how to reduce the sci_strindex.c code to a simple reproducer.

Compiling with -O0 appears to fix it:

scilab -ns -nwni -e "disp(stddir/build/BUILD/scilab-5.4.1/modules/graphics/macros/*.sci','/'));quit;"

    1.    10.    16.    22.    35.    43.    52.    59.

Comment 1 Jakub Jelinek 2013-05-17 20:37:57 UTC
This boils down to a reduced testcase:
extern struct S { double t[2]; } s;

void
foo (int x)
{
  int i;
  for (i = 0; i < x; i++)
    s.t[i] = 0.0;
}

GCC 4.8 figures out that because s.t isn't flexible array member (neither the C99 style struct S { int x; double t[]; }; (note, flexible array member in that case can't be a sole struct member), nor the GNU style struct S { double t[0]; };) and because it is an extern var, valid code which should use the same type for the var definition as for the var declaration can't iterate here more than twice, therefore assumes that x must be <= 2.
If you write extern struct S { int x; double t[]; } s;, or extern struct S { double t[0]; } s;, then the TU definition those could do
struct S { int x; double t[]; } s = { 1, { 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10. } }; (or similarly for t[0]), or if you do:
struct S { double t[0]; };
// struct S { int x; double t[]; };
// struct S { double t[1]; };
// struct S { double t[2]; };
void
bar (struct S *s, int x)
{
  int i;
  for (i = 0; i < x; i++)
    s->t[i] = 0.0;
}
then the first two variants are GNU extension or C99 standard conforming ways which can be e.g. set to malloc/alloca etc. returned pointer with allocated payload, and the trailing arrays at the end of struct are also considered as poor man's flexible array member alternatives.  So, with all 4 of the above definitions bar will loop x times, whatever x is.  But for extern var, as in the scilab case, it looks like invalid code.

Comment 2 Orion Poplawski 2013-05-17 21:00:36 UTC
Thanks Jakub for the analysis.  I guess I never really looked closely at the preprocessed source and there is a lot of macro expansion happening.  So, in the original:

typedef struct
{
#ifdef USE_DYNAMIC_STACK
        double *Stk;
#else
        double Stk[vsiz];
#endif
} STACK_struct;

And USE_DYNAMIC_STACK is enabled on 64-bit compilation (which explains the arch difference).  So it appears that the proper fix is to set USE_DYNAMIC_STACK always.


Note You need to log in before you can comment on or make changes to this bug.