On Wed, Jul 24, 2013 at 07:34:07AM +0200, Michal Simek wrote:
> Microblaze was assign to CLONE_BACKWARDS type where
> parent tid was passed via 3rd argument.
> Microblaze glibc is using 4th argument for it.
>
> Create new CLONE_BACKWARDS3 type where stack_size is passed
> via 3rd argument, parent thread id pointer via 4th,
> child thread id pointer via 5th and tls value as 6th
> argument
I believe this also affects us in musl. What is the motivation for
making a configure option that results in there being two incompatible
syscall ABIs for the same arch? This sounds like a really bad idea...
And how was glibc successfuly using a form that mismatched the
existing kernel? Did nobody ever use/test it? I think the broken
userspace software that was already failing to work due to this
mismatch should simply be fixed rather than adding incompatible kernel
ABI variants.
Rich
Hi Rich,
On 07/24/2013 07:55 AM, Rich Felker wrote:
> On Wed, Jul 24, 2013 at 07:34:07AM +0200, Michal Simek wrote:
>> Microblaze was assign to CLONE_BACKWARDS type where
>> parent tid was passed via 3rd argument.
>> Microblaze glibc is using 4th argument for it.
>>
>> Create new CLONE_BACKWARDS3 type where stack_size is passed
>> via 3rd argument, parent thread id pointer via 4th,
>> child thread id pointer via 5th and tls value as 6th
>> argument
>
> I believe this also affects us in musl. What is the motivation for
> making a configure option that results in there being two incompatible
> syscall ABIs for the same arch?
> This sounds like a really bad idea...
This patch fixes bug which was introduced by Al's patch where he moved
clone implementation from microblaze folder to generic location.
It means I am not creating two incompatible syscalls ABIs but fixing
broken one.
> And how was glibc successfuly using a form that mismatched the
> existing kernel? Did nobody ever use/test it?
We are running LTP syscall tests and there is not LTP test which
was able to find out this mismatch in clone. That's why I haven't
figure it out at that time and ACKed that origin patch.
In my email you can see that I have also asked about tools which should be used
for kernel API testing.
> I think the broken
> userspace software that was already failing to work due to this
> mismatch should simply be fixed rather than adding incompatible kernel
> ABI variants.
The incompatibility is between glibc register setup and the kernel sys_clone
register expectation which doesn't match right now.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On Wed, Jul 24, 2013 at 08:48:27AM +0200, Michal Simek wrote:
> >> Create new CLONE_BACKWARDS3 type where stack_size is passed
> >> via 3rd argument, parent thread id pointer via 4th,
> >> child thread id pointer via 5th and tls value as 6th
> >> argument
> >
> > I believe this also affects us in musl. What is the motivation for
> > making a configure option that results in there being two incompatible
> > syscall ABIs for the same arch?
> > This sounds like a really bad idea...
>
> This patch fixes bug which was introduced by Al's patch where he moved
> clone implementation from microblaze folder to generic location.
> It means I am not creating two incompatible syscalls ABIs but fixing
> broken one.
So this patch is just fixing a regression in the kernel?
> > And how was glibc successfuly using a form that mismatched the
> > existing kernel? Did nobody ever use/test it?
>
> We are running LTP syscall tests and there is not LTP test which
> was able to find out this mismatch in clone. That's why I haven't
> figure it out at that time and ACKed that origin patch.
I would think pthread_create would have broken pretty badly; I
remember early-on in porting musl to microblaze, we had the clone
arguments misordered, and it blew up badly. ;) Perhaps you could just
run some general libc/libpthread level tests to catch things like this
that are hard to measure at the syscall-test level with existing
tests.
Rich
On 07/24/2013 08:59 AM, Rich Felker wrote:
> On Wed, Jul 24, 2013 at 08:48:27AM +0200, Michal Simek wrote:
>>>> Create new CLONE_BACKWARDS3 type where stack_size is passed
>>>> via 3rd argument, parent thread id pointer via 4th,
>>>> child thread id pointer via 5th and tls value as 6th
>>>> argument
>>>
>>> I believe this also affects us in musl. What is the motivation for
>>> making a configure option that results in there being two incompatible
>>> syscall ABIs for the same arch?
>>> This sounds like a really bad idea...
>>
>> This patch fixes bug which was introduced by Al's patch where he moved
>> clone implementation from microblaze folder to generic location.
>> It means I am not creating two incompatible syscalls ABIs but fixing
>> broken one.
>
> So this patch is just fixing a regression in the kernel?
yes.
>>> And how was glibc successfuly using a form that mismatched the
>>> existing kernel? Did nobody ever use/test it?
>>
>> We are running LTP syscall tests and there is not LTP test which
>> was able to find out this mismatch in clone. That's why I haven't
>> figure it out at that time and ACKed that origin patch.
>
> I would think pthread_create would have broken pretty badly; I
> remember early-on in porting musl to microblaze, we had the clone
> arguments misordered, and it blew up badly. ;) Perhaps you could just
> run some general libc/libpthread level tests to catch things like this
> that are hard to measure at the syscall-test level with existing
> tests.
David was running glibc tests and I was also running some pthreads tests
but we have seen the problem only on timer_create tests we have got from
customer.
I found that we should maybe invest our time to open posix testsuite
to get another set of tests we should run.
BTW: Where to get musl package and are there any tests we should regularly run?
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On Wed, Jul 24, 2013 at 09:16:03AM +0200, Michal Simek wrote:
> BTW: Where to get musl package
Source is available from http://www.musl-libc.org/. We also have cross
compilers and binaries for a number of archs here:
https://bitbucket.org/GregorR/musl-cross
However, microblaze was not included since the support in the upstream
GNU packages seemed to be broken/unusable. If this has changed, please
let me know. I'd really like to get microblaze support added added to
the cross-compiler builds.
> and are there any tests we should regularly run?
We have a libc-test package under development here:
http://nsz.repo.hu/git/?p=libc-test
There's nothing musl-specific about the tests; they can also be run
against glibc, and presumably uClibc too. It's still a bit
experimental now, and still pulling in tests we had in various
non-unified test modules into a proper test framework, but it may be
useful.
Rich
On Wed, 24 Jul 2013 08:48:27 +0200 Michal Simek <[email protected]> wrote:
> On 07/24/2013 07:55 AM, Rich Felker wrote:
> > On Wed, Jul 24, 2013 at 07:34:07AM +0200, Michal Simek wrote:
> >> Microblaze was assign to CLONE_BACKWARDS type where
> >> parent tid was passed via 3rd argument.
> >> Microblaze glibc is using 4th argument for it.
> >>
> >> Create new CLONE_BACKWARDS3 type where stack_size is passed
> >> via 3rd argument, parent thread id pointer via 4th,
> >> child thread id pointer via 5th and tls value as 6th
> >> argument
> >
> > I believe this also affects us in musl. What is the motivation for
> > making a configure option that results in there being two incompatible
> > syscall ABIs for the same arch?
> > This sounds like a really bad idea...
>
> This patch fixes bug which was introduced by Al's patch where he moved
> clone implementation from microblaze folder to generic location.
That's important information which was omitted from the changelog.
Please identify the patch which casused this regression (SHA hash and
title), thanks.
Hi Andrew,
On 07/27/2013 01:08 AM, Andrew Morton wrote:
> On Wed, 24 Jul 2013 08:48:27 +0200 Michal Simek <[email protected]> wrote:
>
>> On 07/24/2013 07:55 AM, Rich Felker wrote:
>>> On Wed, Jul 24, 2013 at 07:34:07AM +0200, Michal Simek wrote:
>>>> Microblaze was assign to CLONE_BACKWARDS type where
>>>> parent tid was passed via 3rd argument.
>>>> Microblaze glibc is using 4th argument for it.
>>>>
>>>> Create new CLONE_BACKWARDS3 type where stack_size is passed
>>>> via 3rd argument, parent thread id pointer via 4th,
>>>> child thread id pointer via 5th and tls value as 6th
>>>> argument
>>>
>>> I believe this also affects us in musl. What is the motivation for
>>> making a configure option that results in there being two incompatible
>>> syscall ABIs for the same arch?
>>> This sounds like a really bad idea...
>>
>> This patch fixes bug which was introduced by Al's patch where he moved
>> clone implementation from microblaze folder to generic location.
>
> That's important information which was omitted from the changelog.
>
> Please identify the patch which casused this regression (SHA hash and
> title), thanks.
Title is here and no problem to add it to the patch.
"microblaze: switch to generic fork/vfork/clone"
(sha1: f3268edbe6fe0ce56e62c6d6b14640aeb04864b7)
I will do v2 patch.
I see you have added v1 patch to your repo. What's the reason for that?
I expect just not to forget it, right?
Not sure if Al is on his vacation or not but will be helpful if you
can give me your ACK on that v2 patch and will send pull request directly
to Linus.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform