Re: Add new target: vxworks for xscale

Re: Add new target: vxworks for xscale

[Richard Kenner]

    Given what you have said, and given how very broken the patch was, I
    can only conclude you did not test it beyond "yup, it compiles."  For
    instance, any program that used exception handling would have gotten
    loader errors when actually run on VxWorks.

No, I didn't test it, instead taking it for granted that the submitter did.
I only test patches to make sure they don't break mainline targets, assuming
that the submitter verified they do as they are supposed to.  Indeed I'm
quite surprised it worked on his configuration but not on yours.

It was partly your comment about loader errors that motivated me to get
Olivier involved since it seems like there are different configurations
involved.

Re: Add new target: vxworks for xscale

[Zack Weinberg]

kenner@vlsi1.ultra.nyu.edu (Richard Kenner) writes:

>     Given what you have said, and given how very broken the patch was, I
>     can only conclude you did not test it beyond "yup, it compiles."  For
>     instance, any program that used exception handling would have gotten
>     loader errors when actually run on VxWorks.
>
> No, I didn't test it, instead taking it for granted that the submitter did.
> I only test patches to make sure they don't break mainline targets, assuming
> that the submitter verified they do as they are supposed to.  Indeed I'm
> quite surprised it worked on his configuration but not on yours.

It might have appeared to work in a 3.2 source tree.  The way
exception handling interacts with threads under VxWorks has changed
between 3.2 and the mainline.  You were making changes to get the
patch applied to the mainline, so you should have done a proper retest
on the mainline.

There were other problems with the patch that should have shown up
under 3.2 as well, but they were not as serious.

zw

Re: Add new target: vxworks for xscale

[Olivier Hainque]

Zack Weinberg wrote:
> It might have appeared to work in a 3.2 source tree.

 FWIW, it passes almost 100% of the chapter C tests in the ACAT 2.4 series,
 on a real target. This includes, amongst others, hundreds of tests related to
 Ada tasking and exceptions handling, often intermixed in very convoluted
 ways.

 

Re: Add new target: vxworks for xscale

[Zack Weinberg]

Olivier Hainque <hainque@act-europe.fr> writes:

> Zack Weinberg wrote:
>> It might have appeared to work in a 3.2 source tree.
>
>  FWIW, it passes almost 100% of the chapter C tests in the ACAT 2.4 series,
>  on a real target. This includes, amongst others, hundreds of tests related to
>  Ada tasking and exceptions handling, often intermixed in very convoluted
>  ways.

As far as I know, the Ada runtime does not use the libgcc standard
threading or EH support routines; thus you would not have noticed that
this was broken from that test.

This is why all supported languages are supposed to be built and
tested.

zw

Re: Add new target: vxworks for xscale

[Olivier Hainque]

Zack Weinberg wrote:
> As far as I know, the Ada runtime does not use the libgcc standard
> threading or EH support routines; thus you would not have noticed that
> this was broken from that test.

 That's right. I realize it was not clear from my previous message, but the
 goal was just to provide a datapoint, not to defend against your arguments.

 Now, we actually have everything in place to use the libgcc underlying
 routines. I suggest that we attempt an ACATS run in this sort of setup
 as part of the testing procedure for the next submission, together with the
 changes you suggested to have the port organized the modern way.

 How does that sound ?
  
> This is why all supported languages are supposed to be built and
> tested.

 Right. It's often hard to achieve for VxWorks ports, however.

 We have tools to support the build/download/run/compare process for the
 ACATS series on real boards. We adjusted this once for the SIS simulator,
 which required a specific BSP.

 Are there similar simulators for ARM/variants arounds ?

 Is there an automated way to run the testuite for such targets ?



 
 

Re: Add new target: vxworks for xscale

[Zack Weinberg]

Olivier Hainque <hainque@act-europe.fr> writes:

> Zack Weinberg wrote:
>> As far as I know, the Ada runtime does not use the libgcc standard
>> threading or EH support routines; thus you would not have noticed that
>> this was broken from that test.
>
>  That's right. I realize it was not clear from my previous message,
>  but the goal was just to provide a datapoint, not to defend against
>  your arguments.

That's fair.

>  Now, we actually have everything in place to use the libgcc
>  underlying routines. I suggest that we attempt an ACATS run in this
>  sort of setup as part of the testing procedure for the next
>  submission, together with the changes you suggested to have the
>  port organized the modern way.

This is an excellent idea.

>  Right. It's often hard to achieve for VxWorks ports, however.

Oh, believe me, I know all about the joys of running tests on VxWorks
boards.

Unfortunately I don't have a good answer for you.  I would *like* to
see the C and C++ testsuites run against a physical board, but since
I have never managed to make that work myself, I can't ask it of you.

There is a thing called WindISS which is relatively easy to make work
with gcc and dejagnu, but it's not simulating the real VxWorks
environment, alas.

zw

Re: Add new target: vxworks for xscale

[Olivier Hainque]

Hello Zack,

Zack Weinberg wrote:
> >  Now, we actually have everything in place to use the libgcc
> >  underlying routines. I suggest that we attempt an ACATS run in this
> >  sort of setup as part of the testing procedure for the next
> >  submission, together with the changes you suggested to have the
> >  port organized the modern way.
> 
> This is an excellent idea.

 This will be a very interesting test for us, btw. We are now using the
 GCC support for EH pretty extensively on a couple of platforms, but so far
 have only experimented the DWARF table based underlying machinery. The GNAT
 support for GCC/SJLJ has been introduced very recently, so it needs to be
 exercised :)

 A couple of ACATS tests for Ada ATCs (Asynchronous Transfers of Control) are
 probably bound to fail, but this is a whole different story (a though one,
 btw).
 
> Oh, believe me, I know all about the joys of running tests on VxWorks
> boards.

 Oh, no doubt I believe you :)
 
> Unfortunately I don't have a good answer for you.  I would *like* to
> see the C and C++ testsuites run against a physical board,

 Humm, this is giving me ideas ...
 
> There is a thing called WindISS which is relatively easy to make work
> with gcc and dejagnu, but it's not simulating the real VxWorks
> environment, alas.

 OK. Will see. Thanks for the pointer.

 I can't put all this on the very top of my list *today*, but I'm definitely
 willing to make progress on this issue rapidly. I'll certainly have questions
 along the way :)

 Thanks for your feedback.

 Olivier

Re: Add new target: vxworks for xscale

[Richard Kenner]

    FWIW, it passes almost 100% of the chapter C tests in the ACAT 2.4
    series, on a real target. This includes, amongst others, hundreds of
    tests related to Ada tasking and exceptions handling, often intermixed
    in very convoluted ways.

Yes, but with which EH?  Zack's claim is that the use of GCC EH would cause
link errors.

Re: Add new target: vxworks for xscale

[Richard Kenner]

    You were making changes to get the patch applied to the mainline, so
    you should have done a proper retest on the mainline.

Perhaps, but note that this was a *new port*, so it couldn't have made
anything worse no matter how "broken" it was.

Re: Add new target: vxworks for xscale

[Richard Kenner]

    Why didn't the patch author just submit the patch to gcc-patches for
    review (whether by you, or by the VxWorks maintainers, and with possible
    comments by anyone who happened to read the patch on gcc-patches before it
    was reviewed) in the ordinary manner in the first place?

Because of the "drift" of sources: they were not against the head sources,
but against the 3.2 branch, where they could not be applied due to the state
of that branch.

What I've been working on over the last few weeks has been going
through a large list of such patches, updating them to the head sources,
reviewing and testing them, and installing them.

Re: Add new target: vxworks for xscale

[Zack Weinberg]

kenner@vlsi1.ultra.nyu.edu (Richard Kenner) writes:

>     Why didn't the patch author just submit the patch to gcc-patches for
>     review (whether by you, or by the VxWorks maintainers, and with possible
>     comments by anyone who happened to read the patch on gcc-patches before it
>     was reviewed) in the ordinary manner in the first place?
>
> Because of the "drift" of sources: they were not against the head sources,
> but against the 3.2 branch, where they could not be applied due to the state
> of that branch.
>
> What I've been working on over the last few weeks has been going
> through a large list of such patches, updating them to the head sources,
> reviewing and testing them, and installing them.

The fact remains, that if this patch had been posted for review it
would have been rejected, and we could then have had a discussion
about how it should have been improved, before it went in.  Given what
you have said, and given how very broken the patch was, I can only
conclude you did not test it beyond "yup, it compiles."  For instance,
any program that used exception handling would have gotten loader
errors when actually run on VxWorks.

zw

Re: Add new target: vxworks for xscale

[Richard Kenner]

    I don't like this approach.  As a Fortran maintainer, I could surely be 
    presented with patches thought up or adopted by a global write 
    maintainer.  That doesn't mean I'd just forgo my right to review them ...

No, of course not.  And indeed your comments on such a patch should
be given great weight, as Zack's was in my case.  But that doesn't
mean you (or Zack) would have the right to immediately demand they be
reverted without further discussion.  Your (and Zack's) goal should be
to convince the person who applied the patches that they were wrong and
you were right.  That should normally not be hard (and wasn't in
this case, once the discussion occurred).

    Just applying patches written by someone else without review is utterly 
    wrong, in my not so humble opinion.

That's not what happened in this case.  I not only "reviewed" it, but
made significant changes.  I spent a few hours on this patch, in fact,
and a changed a number of things I didn't like.

Zack had more things he didn't like, but before I was willing to automatically
defer to his opinion, I wanted to give the original author a chance to
address them.  Perhaps there were some things that made this method correct
and Zack wrong.  Perhaps not.  But what's wrong with trying to find out?

Re: Add new target: vxworks for xscale

[Richard Kenner]

    >     I'm sure it appears to work.  That doesn't make it correct.  In any
    >     case, in my capacity as the VxWorks configury maintainer, I don't
    >     want this code in the FSF tree until it's rewritten.  Are you going
    >     to back out the patch?
    >
    > I want to give Olivier some time to comment before we do anything.

    He can do that whether or not the patch is in the FSF tree.  Take it
    out now, please.

I don't appreciate your tone here.

Both of us have blanket write privileges and can't order the other around.

Fundamentally, if we disagree on a patch and can't come to some
agreement, the SC resolves the dispute.

There was no claim that the patch broke anything or even that it didn't work.

We were less than 24 hours after it had been installed.

You had stylistic objections to the patch.  I wanted to defer my opinion
until I saw what response the patch author had to your comments.

Had Olivier agreed with your comments and produced a patch to go on top
of what was there to be consistent with them, I would have put that in.

Had he agreed your criticisms were valid but wasn't able to make an immediate
replacement, I would have reverted it (this is what happened).

But had he *disagreed* with your comments and presented convincing
enough evidence or arguments to support his position, I would have
also disagreed and started a discussion with you to attempt to reach a
concensus as to the right approach.

What was so much of a rush here that we couldn't spend the few extra hours
to go through that process?

Re: Add new target: vxworks for xscale

[Toon Moene]

Richard Kenner wrote:

>     >     I'm sure it appears to work.  That doesn't make it correct.  In any
>     >     case, in my capacity as the VxWorks configury maintainer, I don't
>     >     want this code in the FSF tree until it's rewritten.  Are you going
>     >     to back out the patch?
>     >
>     > I want to give Olivier some time to comment before we do anything.
> 
>     He can do that whether or not the patch is in the FSF tree.  Take it
>     out now, please.
> 
> I don't appreciate your tone here.
> 
> Both of us have blanket write privileges and can't order the other around.

I don't like this approach.  As a Fortran maintainer, I could surely be 
presented with patches thought up or adopted by a global write 
maintainer.  That doesn't mean I'd just forgo my right to review them ...

Just applying patches written by someone else without review is utterly 
wrong, in my not so humble opinion.

-- 
Toon Moene - mailto:toon@moene.indiv.nluug.nl - phoneto: +31 346 214290
Saturnushof 14, 3738 XG  Maartensdijk, The Netherlands
Maintainer, GNU Fortran 77: http://gcc.gnu.org/onlinedocs/g77_news.html
GNU Fortran 95: http://gcc-g95.sourceforge.net/ (under construction)

Re: Add new target: vxworks for xscale

[Joseph S. Myers]

On Sat, 10 May 2003, Richard Kenner wrote:

> You had stylistic objections to the patch.  I wanted to defer my opinion
> until I saw what response the patch author had to your comments.
> 
> Had Olivier agreed with your comments and produced a patch to go on top
> of what was there to be consistent with them, I would have put that in.

Why didn't the patch author just submit the patch to gcc-patches for
review (whether by you, or by the VxWorks maintainers, and with possible
comments by anyone who happened to read the patch on gcc-patches before it
was reviewed) in the ordinary manner in the first place?

-- 
Joseph S. Myers
jsm28@cam.ac.uk

Re: Add new target: vxworks for xscale

[Daniel Jacobowitz]

On Sat, May 10, 2003 at 08:27:18AM -0400, Richard Kenner wrote:
>     >     I'm sure it appears to work.  That doesn't make it correct.  In any
>     >     case, in my capacity as the VxWorks configury maintainer, I don't
>     >     want this code in the FSF tree until it's rewritten.  Are you going
>     >     to back out the patch?
>     >
>     > I want to give Olivier some time to comment before we do anything.
> 
>     He can do that whether or not the patch is in the FSF tree.  Take it
>     out now, please.
> 
> I don't appreciate your tone here.
> 
> Both of us have blanket write privileges and can't order the other around.
> 
> Fundamentally, if we disagree on a patch and can't come to some
> agreement, the SC resolves the dispute.

Between two global write maintainers I'd agree with your conclusion. 
However, this is an area of the compiler with a (two) specific
maintainers, and Zack was speaking as the specific maintainer of the
part of the compiler you changed.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer