Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calling convention mismatch in hyp_view.app causes bus error crash #110

Closed
czietz opened this issue Apr 11, 2019 · 6 comments

Comments

Projects
None yet
4 participants
@czietz
Copy link
Contributor

commented Apr 11, 2019

In the German Atari-Home forum, a user reported a crash when starting hyp_view.app bundled with this snapshot build: https://bintray.com/freemint/freemint/download_file?file_path=snapshots%2F1-19-36cf2b41%2Ffreemint-1-19-36c-02060-tt_falcon_clones.zip
(Screenshot of the error message: https://forum.atari-home.de/index.php?action=dlattach;topic=15155.0;attach=24081;image)

I traced back the bus error: It occurs within the Getcookie() function. Looking at the disassembly, I see:

                             *************************************************************
                             *                           FUNCTION                          
                             *************************************************************
                             undefined  Getcookie ()

        00026f00 4f  ef  ff  f0    lea        (-0x10 ,SP),SP
        00026f04 48  d7  0c  0c    movem.l    {  D2 D3 A2 A3},(SP=>local_10 )
        00026f08 26  00           move.l     D0,D3
        00026f0a 26  48           movea.l    A0,A3

Clearly, it loads its parameters from D0 and A0, i.e., it uses the fastcall calling convention. However, hyp_view.app uses the conventional cdecl calling convention:

        00012364 48  6e  ff  fc    pea        (-0x4 ,A6)
        00012368 2f  3c  44       move.l     #0x44485354 ,-(SP)=>local_30
                 48  53  54
        0001236e 4e  b9  00       jsr        Getcookie .l                                     undefined Getcookie()
                 02  6f  00

This mismatch in calling conventions is the root cause of the crash.

Looking at the source code of libcmini (that is used for hyp_view.app), one can see that Getcookie() indeed uses the fastcall convention: https://github.com/mfro0/libcmini/blob/ab8f44e3c174dd6ae772b9a3d97dceab9a4785fc/sources/getcookie.S#L30. One would have to call getcookie() (with a "g" instead of a "G") to get the cdecl wrapper.

However, this getcookie() was removed by libcmini author @mfro0: "remove getcookie() (as it collides with hypview compilation)".

This unfortunately means that FreeMiNT ships with a broken and crashing hyp_view.app.

@mikrosk

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Ok, so to get this fixed we'd need to call a function which has been removed to fix compilation of the very same project, nice. :-)

Frankly, I don't get why getcookie causes any trouble as hyp_view doesn't call it at all. The "official" (mintlib) signature is Getcookie so if hyp_view is to use both libs (which are supposed to be interchangeable), I'd say libcmini has to provide proper, cdecl, Getcookie version. @mfro0 ?

@th-otto

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

I agree with @mikrosk , that looks like a bug in libcmini. Originally, it had both functions available since Getcookie is also called directly in checkcpu.S with fastcall convention. It should be the other way around: provide a Getcookie function that is compatible with mintlib (using cdecl), and a getcookie function (or maybe even a different name to avoid confusion) that can be called from checkcpu.S

@mfro0

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Sorry gents, was away last week a few thousand miles west and unable to access the internet (not because of lack of it, but because I had to battle with jetlag ;) )

You are right - this appears to be a bug in libcmini.

I didn't yet find time and patience to look into this deeply, but at a glance, the bug seems to be introduced by a nice fellow that provided commit mfro0/libcmini@91c67e4#diff-1bc2b6e7048019bf47e33f364cc0dc0f that apparently inverted the meaning of Getcookie() and getcookie().

I don't blame @th-otto for that, as obviously, there wasn't enough testing of that said patch on my side.

Will try to fix that during the weekend.

@th-otto

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

Well yes i also noticed in the meantime that is was obviously caused by that commit. Apologies for that, and you surely have all the rights to blame me;)

@mfro0

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

Never mind. Mistakes happen ;)

Fixed now in latest libcmini (0.491) release. Getcookie() uses stack calling convention while getcookie() uses FASTCALL (registers) as it is supposed to be.

If the next automated build picks up the latest libcmini release (I think that was what we agreed upon, @mikrosk , right?), hypview should now work again.

mikrosk added a commit to freemint/travis-scripts that referenced this issue Apr 13, 2019

@mikrosk

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

Thanks @czietz for reporting, @mfro0 for fixing so quickly. Fixed in freemint/travis-scripts@c49b3b3, I have restarted Travis so fixed hypview should be available shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.