rsxInit() change brings issue with SDL libraries

[bucanero]

Just noted that recent changes in rsxInit() from https://github.com/ps3dev/PSL1GHT/commit/d0eea6e024a6e86435136b058bc7aaf1cabb0581 , brings some issues when building the SDL_PSL1GHT library from ps3libraries (013-sdl_psl1ght.sh)

e.g., you'll get a too few arguments to function 'rsxInit' error in SDL_PSL1GHTvideo.c

maybe other libs or samples are affected.

[zeldin]

Ugh. That commit mixes a lot of unrelated changes which should have been separate commits...

@shagkur Changing the prototype of the API function rsxInit seems a bit dubious since you'll break all existing code using said API. How about making a new API function rsxInitExt with the extra parameters instead and leaving the old one as it is for compat?

[miigotu]

Too bad it isn't cpp, overloading would be perfect.

[shagkur]

Keep calm. No worries i‘ll fix all corresponding libraries.

miigotu notifications@github.com schrieb am Fr. 10. Juli 2020 um 22:39:

Too bad it isn't cpp, overloading would be perfect.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/ps3dev/PSL1GHT/issues/106#issuecomment-656879461, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADQD7WWCVSIFJPNEU3WVELR2536HANCNFSM4OWWSKGQ .

[shagkur]

Perhaps you can run a build of all ps3libraries against this commit and give me list of the failing ones?

Mike Wiedenbauer mike.wiedenbauer@unblu.com schrieb am Fr. 10. Juli 2020 um 23:04:

Keep calm. No worries i‘ll fix all corresponding libraries.

miigotu notifications@github.com schrieb am Fr. 10. Juli 2020 um 22:39:

Too bad it isn't cpp, overloading would be perfect.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/ps3dev/PSL1GHT/issues/106#issuecomment-656879461, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADQD7WWCVSIFJPNEU3WVELR2536HANCNFSM4OWWSKGQ .

[zeldin]

Well, if you call the function with the new prototype rsxInitExt, you can simply make a compatibility wrapper

gcmContextData* rsxInit(const u32 cmdSize,const u32 ioSize,const void *ioAddress)
{
    gcmContextData *context = NULL;
    rsxInitExt(&context, cmdSize, ioSize, ioAddress);
    return context;
}

and then you don't have to change any libraries (or applications) at all...

[shagkur]

I get your point. As well as i know how it could be solved. But this one is a bit more complicated since i also changed defines by their names. The question here is: What do we maintain? The ps3dev, hence ps3toolchain, psl1ght and the ps3librariies? If so, those are our only responsibilities. And then we still should fix the libraries. Other applications, using librsx directly, not maintained by us we shouldn't bother to stay API backward compatible. And like i wrote before, i broke those lib, i'll fix them.

[ghost]

Thanks @shagkur for fix, @zeldin for merge and @bucanero for reporting. Issue seems to be fixed, I did a successful build of the ps3toolkit without this issue showing up.

[bucanero]

thanks @shagkur 👍 I'm closing this issue; fix merged in https://github.com/zeldin/SDL_PSL1GHT/pull/12

[zeldin]

@shagkur Could you please fix SDL2_PSL1GHT as well? I'd like to merge the develop branch of ps3libraries into master, but right now it does not build due to this breakage...

[shagkur]

@zeldin whats now broken? i already made a PR for rsxInit change and it already got merged!?

[zeldin]

SDL2_PSL1GHT is. Note the "2". (Only the develop branch of ps3libraries builds SDL2_PSL1GHT.)

[shagkur]

Ahh got it. Where is the branch? git url?

[zeldin]

https://github.com/sergiou87/SDL2_PSL1GHT/

[shagkur]

cant fork it (seems to be the same as yours, somehow). github says i've already forked it (the ones from you, for which i already fixed it). Perhaps you can apply this one-liner yourself? the diff from my PR should be still around.

[zeldin]

It looks like he forked the SDL_PSL1GHT repo and then updated it to be SDL2. Just add https://github.com/sergiou87/SDL2_PSL1GHT/ as a remote and you should be able to pull the master branch from there. After you fix the issue, push the fix to a branch in your own fork and then create a PR against sergiou87/SDL2_PSL1GHT.

[shagkur]

Does not work. If i pull/merge the upstream into mine i get a ton of merge conflicts. Not sure how i should solve this.

[zeldin]

Dont pull/merge. Create a new branch based on sergio87/master.

git remote add sergiou87 https://github.com/sergiou87/SDL2_PSL1GHT
git fetch sergiou87
git checkout -b myfixes sergiou87/master

[zeldin]

Then, when you have committed the fix to myfixes and pushed that branch to your own fork, you go to https://github.com/sergiou87/SDL2_PSL1GHT/pulls and press "New pull request", and choose myfixes as the branch to merge into sergiou87/master.

[shagkur]

Done (i guess)

[zeldin]

@shagkur Thanks!

[shagkur]

@zeldin btw i figured a long standing bug with the SPRX call stub, in psl1ght. Its only capable of calling methods where the passed arguments don't exceed GPR10 (above this one, as by specification, arguments passed on the stack).

[zeldin]

Cool. I guess nobody needed to call a function with that many arguments before. :smile:

[shagkur]

probably not. I wanted to use "gcmSetZcull" which takes 12 arguments and always got an assertion with one argument past gpr10. After analyzing the stub asm method (and my little understanding of PPC asm) i figured that we dont pass those args and the SPRX lib just gets undefined data. Trying to fix this by a special EXPORT macro where one can pass the arg count and then i'll copy from the incoming to the outgoing stack. At least thats my idea.

[shagkur]

@zeldin Your change of the RS600_ABI_NAME to linux seems to have a huge impact on resulting binaries. In my case (it's a quite big c++ project) the resulting binary crashes immediately. reverting to the commit before (along with changing back sbrk.c in psl1ght) results in working binaries. Although the rsxtest samples did work with your commit too. Anyways. I strongly believe we need to stick to "aixdesc", as PS3 is AIX ABI compliant and therefor have to figure why the .text duplicate error happens. Perhaps we need to adapt/adjust cell64lv2.h a bit.

small side note: i got the "more than 8 function args to PRX calls" fixed :)

[zeldin]

@shagkur You will need to recompile all files in order to avoid problems.

I don't think the PS3 is "AIX ABI compiliant" any more than it is "Linux ABI compliant", both AIX and (big endian) Linux use ELF ABIv1. The difference here is just that all the non-function-descriptor symbols (starting with .) for functions are dropped, since they are not needed anymore with GNU binutils 2.16 and newer (you can use the function-descriptor symbol as the target of a branch instruction, like in the updated psl1ght code) and the linker will sort things out for you.

[zeldin]

And I know exactly why the .text duplicate error happens. It's because in the old mode for every function foo you get two exported symbols, foo for the function-descriptor, and .foo for the non-function-descriptor entry point. Since there are a number of pre-defined symbols starting with . (such symbols are reserved for internal use by the toolchain), such as .text, .data, .bss and so on, you run a risk of collision for every global function you define. And since the non-fuction-descriptor symbol is not actually needed, the sane thing to do seems to be to remove it.

[zeldin]

@shagkur If you can find a case where the actual binary code generated differs, as opposed to just the symbols, I'd be interested to see it.

[shagkur]

@shagkur You will need to recompile all files in order to avoid problems.

I don't think the PS3 is "AIX ABI compiliant" any more than it is "Linux ABI compliant", both AIX and (big endian) Linux use ELF ABIv1. The difference here is just that all the non-function-descriptor symbols (starting with .) for functions are dropped, since they are not needed anymore with GNU binutils 2.16 and newer (you can use the function-descriptor symbol as the target of a branch instruction, like in the updated psl1ght code) and the linker will sort things out for you.

I already rebuilt toolchain, psl1ght, and my irrlichtPS3 project. and with your latest commit it just crashes (on real HW) reverting to the previous commit makes it work again. I'm not necessarely talking about the PS3 in regards to ABI compliancy. But i think the compiler still changes the code a bit differently and hence it works.

And perhaps chanigng: dot_symbols = !strcmp (rs6000_abi_name, "aixdesc"); to dot_symbols = 0; in cell64lv2.h does the trick. At least from searching thru references it is used to distinguish between the .L and non .L result to generate.

[zeldin]

@shagkur Yes, the dot_symbols effect is the one I'm after. But as I said, there should be no difference in the actual ABI, so again, if you find some case where the generated code is different I'm curious to see it.

BTW, you should not have to revert the PSL1GHT change even with the old toolchain, since it depends only on binutils and not gcc (being inside an asm() and all)...

[shagkur]

@zeldin The only case i can currently offer is my irrlichtP3 (port) project and the example built from it. I'd need to rearrange the example so it is self contained. Then you could fork from repo.

Hmm, but was the "." remove in sbrk.c not necessary due to the ABI change of the toolchain? Or what was the reason?

[zeldin]

Well, if you feel inclined to do so I'd be happy to take a closer look at it.

Removal of the "." in sbrk.c is necessary due to the "."-symbol no longer being available, but even when the "."-symbol is available, you can make the call to the symbol without the "." because that is a feature in ld. Thus the new code without the "." works with both toolchain versions and does not need to be reverted if you switch back.

[shagkur]

Well, if you feel inclined to do so I'd be happy to take a closer look at it.

Removal of the "." in sbrk.c is necessary due to the "."-symbol no longer being available, but even when the "."-symbol is available, you can make the call to the symbol without the "." because that is a feature in ld. Thus the new code without the "." works with both toolchain versions and does not need to be reverted if you switch back.

Okay. this with the '.' symbol and how it's handled it didn't know. I just wanted to make sure i'm at a point where i know it did work. But i'm happy to revert to yours (i just did it locally tho) Would be great if you could take a closer look at it. I'll prepare the example to have all necessary data files in the repo. And will update you

[zeldin]

Thanks!

[shagkur]

@zeldin Here we go.... clone/fork this one: https://github.com/shagkur/irrlichtPS3.git and build example/2 (as well as the whole library)

[zeldin]

@shakgur Thanks. I get this while compiling though:

Unable to load Cg, aborting.
Please install Cg toolkit and/or set the path (i.e. LD_LIBRARY_PATH) to the shared library accordingly.

"Cg toolkit" appears to be unmaintained, unsupported, and closed source X86 code. Is there an opensource alternative, or could you perhaps provide the relevant build products somewhere?

[shagkur]

@zeldin What Os do you use? I'm on ubuntu and i just did sudo apt-get install -y nvidia-cg-toolkit. But this would also mean the rsxtest sample of psl1ght does not compile for you either?

[zeldin]

@shagkur Gentoo/ppc64. Correct, the rsxtest sample does not build, but that is not needed to build and install PSL1GHT.

[shagkur]

@zeldin okay. Sure it's not needed to get psl1ght built, but to know whether the stuff works or not it does matter ;) So if i get it right there's no way for you to build irrlichtPS3 due to the missing cg toolkit on your platform :(

[zeldin]

Well, if you give me the build products generated with Cg toolkit, I can build the rest. We are looking for an issue with gcc, not Cg toolkit, so I don't expect I shall need to rebuild those parts. Going forward it would be nice with an open source solution to build the rsxtest sample of course, but I guess that is a separate issue. :smile_cat:

[shagkur]

That's true. I'll get you the fragments needed to build the library. Yes, would be nice, but there's no decent Opensource cg compiler out there as far as know.

[shagkur]

irrlichtPS3shaders.zip

[shagkur]

Put the files into the build folder of the library.

[zeldin]

Thanks. Now I can build examples/2. Will investigate.

[shagkur]

Thank your very much!

[zeldin]

Hm, interresting. With the new gcc the .text section is smaller, but instead the .got section is larger. The total size seems to be the same...

[zeldin]

The difference appears to be that in one case globals are found by loading their address from the .got (ld X(r2), with a R_PPC64_TOC16_DS relocation), an in the other by adding an offset to r2 (addis r2,X and addi X with R_PPC64_TOC16_HA and R_PPC64_TOC16_LO). Shouldn't matter as the resulting pointer should be the same either way, but I'll try to negate this change and see if anything else differs...

[shagkur]

Well to me it seems by changing the abi name and in gcc configure addin -melf64ppc results in different build results. Keep in mind that the original PS3 SDK still is on gcc 4.1.1 and probably the underlaying FW OS is kinda relies on some sort of ABI compliancy. But that's just wild guessing by me :smiley:

[zeldin]

We've always been incompatible with the FW OS ABI (which uses 32 bit pointers, for examples). That why we need wrappers.

[zeldin]

The TOC thing seems to be because large TOC support is now default (you've always had the options to turn it on), but I don't see why it should break anything. As I said, I'll try turning it off and see if anything else differs.

[shagkur]

Yeah sure go on. I guess, nowadays, you know more and better about gcc toolchain build tweaking. I did that things at that time myself. But this is ages ago hence i'm quite happy you take a look at it.