search

bebbo / gcc

Bebbo's gcc-6-branch for m68k-amigaos
GNU General Public License v2.0
33 stars 11 forks source link

-flto causes crash #60

Closed mheyer32 closed 5 years ago

mheyer32 commented 5 years ago

I have to report a 'mystery' bug that seems to have been introduced with LTO.

It took me quite a while to figure out and I don't know how to reproduce this with a smaller example. What it boils down to is that under some unknown circumstances the compiler seems to forget that the return value of a certain function resides in D0 and falsely assumes it is in A3.

https://github.com/mheyer32/scummvm-amigaos3/blob/amigaos3_2.1/engines/sci/sound/music.cpp#L139

if (_pMidiDrv && !_pMidiDrv->open()) {
        _pMidiDrv->setTimerCallback(this, &miditimerCallback);
        _dwTempo = _pMidiDrv->getBaseTempo();
    }

What happens here is that the jump into _pMidiDrv->open() seems to jump into NULL and we end up crashing in PC 0x0000000C.

pMidiDrv is being created up here https://github.com/mheyer32/scummvm-amigaos3/blob/amigaos3_2.1/engines/sci/sound/music.cpp#L116

        if (g_sci->getPlatform() == Common::kPlatformAmiga || platform == Common::kPlatformMacintosh)
            _pMidiDrv = MidiPlayer_AmigaMac_create(_soundVersion);
        else

MidiPlayer *MidiPlayer_AmigaMac_create(SciVersion version) {
    return new MidiPlayer_AmigaMac(version);
}

The failing ASM sequence looks like this ( I reordered for sequential reading):

// create the MIDI driver
  20f0f6:   102a 0008       move.b 8(a2),d0
  20f0fa:   2f00            move.l d0,-(sp)
  20f0fc:   4eb9 0021 e488  jsr 21e488 <Sci::MidiPlayer_AmigaMac_create(Sci::SciVersion)>
// the culprit:
  20f102:   254b 000e       move.l a3,14(a2)        >>> I believe this wants to store the returned pointer into this->pMidiDrv
                                                                         >>> but why is it using A3 not D0?
  20f106:   588f            addq.l #4,sp
// this must be the "if ( _pMidiDrv &&" part
  20f108:   200b            move.l a3,d0
  20f10a:   6700 fdf0       beq.w 20eefc <Sci::SciMusic::init()+0x1bc>  //check for NULL and bail out if necessary
  20f10e:   6000 fe30       bra.w **20ef40** <Sci::SciMusic::init()+0x200>  >> jump to call into _pMidiDrv->open()

 jump to call into _pMidiDrv->open()
  **20ef40**:   224b            movea.l a3,a1    // pMidiDrv
  20ef42:   2051            movea.l (a1),a0          // get VTABLE?
  20ef44:   2254            movea.l (a4),a1
  20ef46:   2f29 00de       move.l 222(a1),-(sp)
  20ef4a:   2f0b            move.l a3,-(sp)
  20ef4c:   2068 0014       movea.l 20(a0),a0      // fetch pMidiDrv->open() address
  20ef50:   4e90            jsr (a0)                       //   jump into Nirvana!
  20ef52:   508f            addq.l #8,sp
  20ef54:   4a80            tst.l d0
  20ef56:   66a4            bne.s 20eefc <Sci::SciMusic::init()+0x1bc>

Just inserting some print statement(s) causes the code to get reordered and the bug goes away:


  20f246:   102a 0008       move.b 8(a2),d0
  20f24a:   2f00            move.l d0,-(sp)
  20f24c:   4eb9 0021 e5d8  jsr 21e5d8 <Sci::MidiPlayer_AmigaMac_create(Sci::SciVersion)>
// This time its right!
  20f252:   2540 000e       move.l d0,14(a2)     >>>>>>> Use D0 and store in this->pMidiDrv
  20f256:   4fef 0014       lea 20(sp),sp
  20f25a:   6000 fd72       bra.w **20efce** <Sci::SciMusic::init()+0x19e>

>>>>>>>>>
// first part is a printf() + flush
  20efce:   4878 0092       pea 92 <_exit+0x2a>
  20efd2:   4879 0018 0d88  pea 180d88 <_ZZN3Sci8SciMusic4initEvE8__func__.lto_priv.5537>
  20efd8:   487a fe20       pea 20edfa <Sci::SciMusic::~SciMusic()+0x6e>(pc)
  20efdc:   4e94            jsr (a4)
  20efde:   2079 0000 6ecc  movea.l 6ecc <Common::NEResources::getResource(Common::WinResourceID const&, Common::WinResourceID const&)+0xc4>,a0
  20efe4:   2f28 0004       move.l 4(a0),-(sp)
  20efe8:   4e93            jsr (a3)

// pull pMidiDrv from 'this'
  20efea:   206a 000e       movea.l 14(a2),a0  // get pMidiDrv
  20efee:   4fef 0010       lea 16(sp),sp
// this must be the "if ( _pMidiDrv &&" part
  20eff2:   2008            move.l a0,d0
  20eff4:   6700 0116       beq.w 20f10c <Sci::SciMusic::init()+0x2dc>

  20eff8:   2250            movea.l (a0),a1    // VTABLE
  20effa:   2c55            movea.l (a5),a6
  20effc:   2f2e 00de       move.l 222(a6),-(sp)
  20f000:   2f08            move.l a0,-(sp)
  20f002:   2069 0014       movea.l 20(a1),a0   // pointer to open() function
  20f006:   4e90            jsr (a0)                      // jump!
  20f008:   508f            addq.l #8,sp
  20f00a:   4a80            tst.l d0
  20f00c:   6600 00fe       bne.w 20f10c <Sci::SciMusic::init()+0x2dc>

The CXX and LDFLAGS are shown in config.mk after the configure step:

CXXFLAGS :=   -W -Wno-unused-parameter -Wno-unused-function -Wno-empty-body -std=c++11 -g -fvar-tracking-assignments -I./camd-37.1/development/include -fdebug-prefix-map=/home/matze/scummvm-amigaos3=source -noixemul -m68030 -flto -fno-rtti -fno-exceptions -fshort-enums -fstack-reuse=all -fstrict-aliasing -fomit-frame-pointer -Ofast -Wuninitialized

LDFLAGS += -g -noixemul -m68030 -flto -W -Wno-unused-parameter -Wno-unused-function -Wno-empty-body -std=c++11 -g -fvar-tracking-assignments -I./camd-37.1/development/include -fdebug-prefix-map=/home/matze/scummvm-amigaos3=source -noixemul -m68030 -flto -fno-rtti -fno-exceptions -fshort-enums -fstack-reuse=all -fstrict-aliasing -fomit-frame-pointer -Ofast -Wuninitialized

To reproduce check out https://github.com/mheyer32/scummvm-amigaos3/tree/amigaos3_2.1

run (provide your preferred path to --with-amiga-prefix ) ./configure --host=m68k-amigaos --disable-all-engines --enable-engine=sci --disable-mt32emu --enable-release --enable-optimizations --disable-hq-scalers --disable-translation --enable-c++11 --disable-updates --disable-highres --with-amiga-prefix=/media/sf_Amiga/ScummVM

the build via make amigaos3dist -j8

I am testing by just attempting to start "Space Quest 1 - The Sarian Encounter, Enhanced Version" for the AMIGA

bebbo commented 5 years ago

I followed your steps and do not get your code...

 22f302:    102a 0008       move.b 8(a2),d0
  22f306:   2f00            move.l d0,-(sp)
  22f308:   4eb9 0021 3d9c  jsr 213d9c <__ZN3Sci26MidiPlayer_AmigaMac_createENS_10SciVersionE>
  22f30e:   2540 000e       move.l d0,14(a2)
  22f312:   588f            addq.l #4,sp
    if (_pMidiDrv && !_pMidiDrv->open()) {
  22f314:   6700 fe5e       beq.w 22f174 <__ZN3Sci8SciMusic4initEv+0x1b0>
  22f318:   6000 fe9e       bra.w 22f1b8 <__ZN3Sci8SciMusic4initEv+0x1f4>
mheyer32 commented 5 years ago

Weird. I'll try a squeaky clean build in a separate directory tonight and see what happens. I haven't updated the toolchain in the last few days/week - I am not aware of any relevant changes in there, though?

mheyer32 commented 5 years ago

I just tested again with a clean repository and also updated the toolchain. I still get the A3 sequence:

  20f0fc:   4eb9 0021 e488  jsr 21e488 <Sci::MidiPlayer_AmigaMac_create(Sci::SciVersion)>
  20f102:   254b 000e       move.l a3,14(a2)
  20f106:   588f            addq.l #4,sp
  20f108:   200b            move.l a3,d0
...

Are you sure you checked out the origin/amigaos3_2.1 branch?

The resulting unstripped executable is 15057128 bytes large.

bebbo commented 5 years ago

using cygwin:

> git branch
* amigaos3_2.1
  master
> make clean
...
> make amigaos3dist 
...
> ll scummvm
-rwxrwxr-x+ 1 stefan Kein 15180288 Jan 30 19:58 scummvm
> m68k-amigaos-objdump.exe -d scummvm >scummvm.txt
> grep 'move.l a3,14(a2)' scummvm.txt
   ec590:       254b 000e       move.l a3,14(a2)
   ec622:       254b 000e       move.l a3,14(a2)
  10defe:       254b 000e       move.l a3,14(a2)
> grep 'MidiPlayer_AmigaMac_create' scummvm.txt
00213d9c <__ZN3Sci26MidiPlayer_AmigaMac_createENS_10SciVersionE>:
  22f308:       4eb9 0021 3d9c  jsr 213d9c <__ZN3Sci26MidiPlayer_AmigaMac_createENS_10SciVersionE>

So can't find that error here...

... I'll try a different plattform now...

mheyer32 commented 5 years ago

As promised, its a mystery bug :-)

I'm compiling under Ubuntu 16.04

matze@osboxes:~/temp/scummvm-amigaos3$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/5/lto-wrapper
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 5.4.0-6ubuntu1~16.04.11' --with-bugurl=file:///usr/share/doc/gcc-5/README.Bugs --enable-languages=c,ada,c++,java,go,d,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-5 --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-vtable-verify --enable-libmpx --enable-plugin --with-system-zlib --disable-browser-plugin --enable-java-awt=gtk --enable-gtk-cairo --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-5-amd64/jre --enable-java-home --with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-5-amd64 --with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-5-amd64 --with-arch-directory=amd64 --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --enable-objc-gc --enable-multiarch --disable-werror --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.11)
bebbo commented 5 years ago

maybe it's 64bit related - maybe it's my optimizer...

... could you test it with -fbbb=-?

mheyer32 commented 5 years ago

-fbbb=- makes a difference

20ffa6: 4eb9 0021 f3a8 jsr 21f3a8 <Sci::MidiPlayer_AmigaMac_create(Sci::SciVersion)> 20ffac: 2540 000e move.l d0,14(a2) 20ffb0: 588f addq.l #4,sp 20ffb2: 6700 fdf8 beq.w 20fdac <Sci::SciMusic::init()+0x1b8> 20ffb6: 6000 fe38 bra.w 20fdf0 <Sci::SciMusic::init()+0x1fc>

bebbo commented 5 years ago

ok - then -flto seems to reuse instructions... my guess: the offending switch is r.

could you try

  1. -fbbb=r and maybe the error is there too?
  2. if not try all switches without r and maybe the error is gone?
mheyer32 commented 5 years ago

I ran out of time, will try tonight.

Just to make sure, -fbbb=-r would be "all switches without r", right?

bebbo commented 5 years ago

Just to make sure, -fbbb=-r would be "all switches without r", right?

sorry - is not supported yet...

use -fbbb=abcefilmpsz for all without r

mheyer32 commented 5 years ago

You're definitely onto something here. -fbbb=r introduces the bug -fbbb=abcefilmpsz does not have the bug

make amigaos3dist -j8
m68k-amigaos-objdump -m m68k:68030 -d -C scummvm > scummvm_lto.asm
grep -a10 'MidiPlayer_AmigaMac_create' scummvm_lto.asm
bebbo commented 5 years ago

which libs do you have installed? Guess that's a reason for different compilation results too.

mheyer32 commented 5 years ago

I’m not using any libs other than the ones coming with the Amiga toolchain. If you’re talking about the libraries on the host, could you provide me with a command line that lists the one you’re interested in?

bebbo commented 5 years ago

I have a bunch of additional libs^^:

libfreetype.a  libpng.a    libportaudio.a  libSDL_ttf.a  libvorbisenc.a   libz.a
libogg.a       libpng16.a  libregex.a      libvorbis.a   libvorbisfile.a ...

moving these out of the way...

bebbo commented 5 years ago

there it is:

$ grep 'move.l a3,14(a2)' scummvm.txt
   ee346:       254b 000e       move.l a3,14(a2)
   ee3d8:       254b 000e       move.l a3,14(a2)
  113138:       254b 000e       move.l a3,14(a2)
  20d9d0:       254b 000e       move.l a3,14(a2)
  20dac2:       254b 000e       move.l a3,14(a2)
  20db46:       254b 000e       move.l a3,14(a2)
  20db74:       254b 000e       move.l a3,14(a2)
  20dba0:       254b 000e       move.l a3,14(a2)
  20dbdc:       254b 000e       move.l a3,14(a2)

$ grep 'MidiPlayer_AmigaMac_create' scummvm.txt
  20dbd6:       4eb9 0021 ce7c  jsr 21ce7c <__ZN3Sci26MidiPlayer_AmigaMac_createENS_10SciVersionE>
0021ce7c <__ZN3Sci26MidiPlayer_AmigaMac_createENS_10SciVersionE>:

20dbd6 and 20dbdc

mheyer32 commented 5 years ago

Oh. Mhhm, check if the configure script lists those as available... I think it is looking for them, but usually cannot find them in my environment and thus does not use them.

The bug itself seems very elusive. It got first introduced when I added code in a completely different place, completely unrelated to the music stuff. In fact, I haven't touched any of the music code. Yet it was the music code that started to break.

This is how configure looks here:

matze@osboxes:~/temp/scummvm-amigaos3$ ./configure --host=m68k-amigaos --disable-all-engines --enable-engine=sci --disable-mt32emu --enable-release --enable-optimizations --disable-hq-scalers --with-amiga-prefix=/media/sf_Amiga/ScummVM --disable-translation --enable-c++11  --disable-updates --disable-highres
Running ScummVM configure...
Looking for C++ compiler... m68k-amigaos-g++
Checking for compiler version... 6.5.0b, ok
Building as C++11... yes
Checking best debug mode... -g + var tracking
Checking for whether C++ compiler accepts -Wglobal-constructors... no
Checking for whether C++ compiler accepts -Wno-undefined-var-template... no
Checking for whether C++ compiler accepts -Wno-pragma-pack... no
Checking endianness... big
Checking 64-bitness... no
Type with 1 byte... char
Type with 2 bytes... short
Type with 4 bytes... int
Type with 8 bytes... long long
Alignment required... no
Checking host CPU architecture... unknown (m68k)
Checking hosttype... amigaos3
Cross-compiling to m68k-amigaos
Checking if host is POSIX compliant... no
Checking whether to have a verbose build... no
Checking whether building plugins was requested... no
Checking for pkg-config... yes
WARNING: When cross-compiling PKG_CONFIG_LIBDIR must be set to the location of the .pc files for the target
Checking for Ogg... no
Checking for Vorbis... no
Checking for Tremor... no
Checking for OPL2LPT... no
Checking for FLAC >= 1.0.1... no
Checking for MAD... no
Checking for ALSA >= 0.9... no
Checking for libjpeg >= v6b... no
Checking for PNG >= 1.2.8... no
Checking for libtheoradec >= 1.0... skipping. no vorbis
Checking for libfaad... no
Checking for SEQ MIDI... no
Checking for sndio... no
Checking for TiMidity... no
Checking for zlib... no
Checking for libmpeg2 >= 0.4.0... no
Checking for liba52... no
Checking for libcurl... no
Cloud integration... no
Checking for FluidSynth... no
Checking for readline... skipping (text console disabled)
Checking for libunity... no
Looking for freetype-config... none found!
Checking for FreeType2... no
Checking for OpenGL... no
Building translation support... no
Building taskbar integration support... no
Building system dialogs support... no
Building Bink video support... no
Building updates support... no
Backend... amigaos3, savegame timestamp, Nuked OPL emulator

Engines (builtin):
    SCI [SCI 0-1.1 games]

Engines Skipped:
    SCUMM 
    Access 
    ADL 
    AGI 
    AGOS 
    Lord Avalot d'Argent 
    Beavis and Butthead in Virtual Stupidity 
    Blade Runner 
    CGE 
    CGE2 
    Chewy: Esc from F5 
    Cinematique evo 1 
    Magic Composer 
    Cinematique evo 2 
    Lost Eden 
    Macromedia Director 
    Dungeon Master 
    Dragon History 
    Drascula: The Vampire Strikes Back 
    Dreamweb 
    Full Pipe 
    ScummGlk Interactive Fiction games 
    UFOs 
    Gobli*ns 
    Groovie 
    Hopkins FBI 
    Hugo Trilogy 
    Illusions Engine 
    Kyra 
    Labyrinth of Time 
    The Last Express 
    Lilliput 
    Lure of the Temptress 
    MacVenture 
    MADE 
    MADS 
    Mohawk 
    Mortevielle 
    Mutation of JB 
    Neverhood 
    Parallaction 
    The Journeyman Project: Pegasus Prime 
    Pink Panther 
    Plumbers Don't Wear Ties 
    The Prince and The Coward 
    Flight of the Amazon Queen 
    SAGA 
    SCI [SCI32 games]
    The Lost Files of Sherlock Holmes 
    Beneath a Steel Sky 
    Sludge 
    Star Trek 25th Anniversary/Judgment Rites 
    Mission Supernova 
    Broken Sword 
    Broken Sword II 
    Broken Sword 2.5 
    Teen Agent 
    TestBed: the Testing framework 
    Tinsel 
    Starship Titanic 
    3 Skulls of the Toltecs 
    Tony Tough and the Night of Roasted Moths 
    Toonstruck 
    Touche: The Adventures of the Fifth Musketeer 
    TsAGE 
    Bud Tucker in Double Trouble 
    Voyeur 
    WAGE 
    Wintermute 
    World of Xeen 
    Z-Vision 

Creating config.h
Creating config.mk
Creating engines/engines.mk
Creating engines/plugins_table.h
bebbo commented 5 years ago

at least it's only a dumb one...

please test

mheyer32 commented 5 years ago

You fixed it! :-)

Closing.