Program that segfaults with -Ofast

[Mason]

On 07/01/2018 22:27, Christer Solskogen wrote:
> On 05.01.2018 14:49, Mason wrote:
>> On 05/01/2018 14:09, Mason wrote:
>>
>>> On 05/01/2018 12:58, Christer Solskogen wrote:
>>>
>>>> ==7821== Conditional jump or move depends on uninitialised value(s)
>>>> ==7821==    at 0x29478C: read_kickstart (memory.cpp:581)
>>>> ==7821==    by 0x29478C: read_kickstart_version(uae_prefs*) (memory.cpp:1151)
>>>> ==7821==    by 0x28E243: fixup_prefs(uae_prefs*, bool) (main.cpp:195)
>>>> ==7821==    by 0x1AB34F: cfgfile_load(uae_prefs*, char const*, int*, int, int) (cfgfile.cpp:3969)
>>>> ==7821==    by 0x2F593B: target_cfgfile_load(uae_prefs*, char const*, int, int) (amiberry.cpp:527)
>>>> ==7821==    by 0x28F927: parse_cmdline_and_init_file (main.cpp:608)
>>>> ==7821==    by 0x28F927: real_main2 (main.cpp:686)
>>>> ==7821==    by 0x28F927: real_main(int, char**) (main.cpp:784)
>>>> ==7821==    by 0x151D97: main (amiberry.cpp:920)
>>>> ==7821==
>>>> ==7821== Conditional jump or move depends on uninitialised value(s)
>>>> ==7821==    at 0x29479C: read_kickstart (memory.cpp:584)
>>>> ==7821==    by 0x29479C: read_kickstart_version(uae_prefs*) (memory.cpp:1151)
>>>> ==7821==    by 0x28E243: fixup_prefs(uae_prefs*, bool) (main.cpp:195)
>>>> ==7821==    by 0x1AB34F: cfgfile_load(uae_prefs*, char const*, int*, int, int) (cfgfile.cpp:3969)
>>>> ==7821==    by 0x2F593B: target_cfgfile_load(uae_prefs*, char const*, int, int) (amiberry.cpp:527)
>>>> ==7821==    by 0x28F927: parse_cmdline_and_init_file (main.cpp:608)
>>>> ==7821==    by 0x28F927: real_main2 (main.cpp:686)
>>>> ==7821==    by 0x28F927: real_main(int, char**) (main.cpp:784)
>>>> ==7821==    by 0x151D97: main (amiberry.cpp:920)
>>>> ==7821==
>>>> ==7821== Invalid read of size 4
>>>> ==7821==    at 0x28E2AC: fixup_prefs(uae_prefs*, bool) (main.cpp:207)
>>>> ==7821==    by 0x1AB34F: cfgfile_load(uae_prefs*, char const*, int*, int, int) (cfgfile.cpp:3969)
>>>> ==7821==    by 0x2F593B: target_cfgfile_load(uae_prefs*, char const*, int, int) (amiberry.cpp:527)
>>>> ==7821==    by 0x28F927: parse_cmdline_and_init_file (main.cpp:608)
>>>> ==7821==    by 0x28F927: real_main2 (main.cpp:686)
>>>> ==7821==    by 0x28F927: real_main(int, char**) (main.cpp:784)
>>>> ==7821==    by 0x151D97: main (amiberry.cpp:920)
>>>> ==7821==  Address 0x29bb8 is not stack'd, malloc'd or (recently) free'd
>>>
>>> I'm starting to think I'm not looking at the correct source code :-)
>>
>> Indeed! You are using the 'dev' branch. Doh! :-)
>>
>> 	uae_u8 mem[32] = { 0 };
>> 	read_kickstart(z, mem, sizeof mem, 0, 0);
>>
>> So memory.cpp:1151 = read_kickstart() with a size of 32.
>>
>>    oldpos = zfile_ftell (f);
>>    i = zfile_fread (buffer, 1, 11, f);
>>    if (!memcmp(buffer, "KICK", 4)) {
>>
>> You should print the value of 'oldpos' and 'i' because it looks like
>> the fread call could not find 4 bytes to load into 'buffer'...
>> (The UB at memory.cpp:581 and memory.cpp:584)
>>
> 
> Fixed!
> 
> Wanna know something funny?
> Having a printf fixed the segfault. Without it, it would crash.
> So the workaround in read_kickstart (memory.cpp) was to add
> "volatile int i" instead of "int i".
> 
> Just one simple printf("Value of i: %d\n", i); was enough to not segfault.

As Andrew points out, making i volatile is most definitely not a proper fix.
At best, it is sweeping the issue under the rug... until a future version of
gcc stumbles upon the issue again. (One sure way to trigger all kinds of
nasty UB is to enable LTO.)

I don't have a Rpi to test live code, which is why I ask you to run a few
tests. I can help track down the root issue if you want me to.

Can you apply the following patch and report what was printed?

diff --git a/src/memory.cpp b/src/memory.cpp
index b91d6f224f2e..127f720e6ac5 100644
--- a/src/memory.cpp
+++ b/src/memory.cpp
@@ -578,6 +578,8 @@ static int read_kickstart (struct zfile *f, uae_u8 *mem, int size, int dochecksu
   }
   oldpos = zfile_ftell (f);
   i = zfile_fread (buffer, 1, 11, f);
+  printf("A oldpos=%d i=%d size=%d name=%s zipname=%s size=%llu seek=%llu\n",
+         oldpos, i, size, f->name, f->zipname, f->size, f->seek);
   if (!memcmp(buffer, "KICK", 4)) {
     zfile_fseek (f, 512, SEEK_SET);
     kickdisk = 1;
@@ -600,6 +602,8 @@ static int read_kickstart (struct zfile *f, uae_u8 *mem, int size, int dochecksu
        }
 
   i = zfile_fread (mem, 1, size, f);
+  printf("B oldpos=%d i=%d size=%d name=%s zipname=%s size=%llu seek=%llu\n",
+         oldpos, i, size, f->name, f->zipname, f->size, f->seek);
 
   if (kickdisk && i > ROM_SIZE_256)
     i = ROM_SIZE_256;


Regards.