Patch pending review

Patch pending review

[Joseph S. Myers]

The patch <http://gcc.gnu.org/ml/gcc-patches/2004-06/msg01966.html>
(restoring format checking of warning(), error(), pedwarn(), sorry()) is
pending review.

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/
    jsm@polyomino.org.uk (personal mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)

Re: Patch pending review

[Gabriel Dos Reis]

You Wrote Joseph S. Myers
> The patch <http://gcc.gnu.org/ml/gcc-patches/2004-06/msg01966.html>
> (restoring format checking of warning(), error(), pedwarn(), sorry()) is
> pending review.

The patch is OK.

Re: Patch pending review

[Zack Weinberg]

"Joseph S. Myers" <jsm@polyomino.org.uk> writes:

> My patch <http://gcc.gnu.org/ml/gcc-patches/2004-10/msg00425.html>
>
>> libcpp:
>> 2004-10-05  Joseph S. Myers  <jsm@polyomino.org.uk>
>> 
>> 	* errors.c (_cpp_begin_message): Print "error: " for errors.
>> 
>> gcc/testsuite:
>> 2004-10-05  Joseph S. Myers  <jsm@polyomino.org.uk>
>> 
>> 	* gcc.dg/cpp/error-1.c: New test.
>
> is currently pending review.

This is OK.

zw

Patch pending review

[Joseph S. Myers]

My patch <http://gcc.gnu.org/ml/gcc-patches/2004-10/msg00425.html>

> libcpp:
> 2004-10-05  Joseph S. Myers  <jsm@polyomino.org.uk>
> 
> 	* errors.c (_cpp_begin_message): Print "error: " for errors.
> 
> gcc/testsuite:
> 2004-10-05  Joseph S. Myers  <jsm@polyomino.org.uk>
> 
> 	* gcc.dg/cpp/error-1.c: New test.

is currently pending review.

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/
    jsm@polyomino.org.uk (personal mail)
    joseph@codesourcery.com (CodeSourcery mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)

Patch pending review

[Joseph S. Myers]

The bit-field patch
<http://gcc.gnu.org/ml/gcc-patches/2004-03/msg01300.html> is still pending
review (cf.  
<http://gcc.gnu.org/ml/gcc-patches/2004-04/threads.html#00105>).

-- 
Joseph S. Myers
jsm@polyomino.org.uk

Patch pending review

[Joseph S. Myers]

http://gcc.gnu.org/ml/gcc-patches/2004-03/msg01300.html
(Bit-field patch, resurrected)
is still pending review.

-- 
Joseph S. Myers
jsm@polyomino.org.uk

Patch pending review

[Joseph S. Myers]

http://gcc.gnu.org/ml/gcc-patches/2004-03/msg01300.html
(Bit-field patch, resurrected)
is pending review.

-- 
Joseph S. Myers
jsm@polyomino.org.uk

Patch pending review

[Joseph S. Myers]

http://gcc.gnu.org/ml/gcc-patches/2004-01/msg03124.html
(getting diagnostic.def messages into gcc.pot)

        * diagnostic.h (DEFINE_DIAGNOSTIC_KIND): Change parameter M to
        msgid.

is currently pending review.

-- 
Joseph S. Myers
jsm@polyomino.org.uk

Re: Patch pending review

[Gabriel Dos Reis]

"Joseph S. Myers" <jsm@polyomino.org.uk> writes:

| http://gcc.gnu.org/ml/gcc-patches/2004-01/msg03124.html
| (getting diagnostic.def messages into gcc.pot)
| 
|         * diagnostic.h (DEFINE_DIAGNOSTIC_KIND): Change parameter M to
|         msgid.
| 
| is currently pending review.

OK.

-- Gaby

Re: Patch pending review

[Gabriel Dos Reis]

"Joseph S. Myers" <jsm@polyomino.org.uk> writes:

| http://gcc.gnu.org/ml/gcc-patches/2004-01/msg03124.html
| (getting diagnostic.def messages into gcc.pot)
| 
|         * diagnostic.h (DEFINE_DIAGNOSTIC_KIND): Change parameter M to
|         msgid.
| 
| is currently pending review.

OK.

-- Gaby

Patch pending review

[Joseph S. Myers]

http://gcc.gnu.org/ml/gcc-patches/2004-01/msg03124.html
(getting diagnostic.def messages into gcc.pot)

        * diagnostic.h (DEFINE_DIAGNOSTIC_KIND): Change parameter M to
        msgid.

is currently pending review.

-- 
Joseph S. Myers
jsm@polyomino.org.uk

Patch pending review

[Franz Sirl]

Hi,

this patch fixes one of the 2 showstopper bugs on powerpc-linux-gnu for 3.1 
and still needs review from an reload expert:

<http://gcc.gnu.org/ml/gcc-patches/2002-02/msg01568.html>

Thanks,
Franz.

Re: Patch pending review

[Stan Shebs]

Franz Sirl wrote:
> 
> Hi,
> 
> this patch:
> 
>         [PATCH] Fix libobjc lib install (libobjc/4039)
>         <http://gcc.gnu.org/ml/gcc-patches/2002-02/msg00043.html>
> 
> needs review.

Sorry, this one got by me.  Yes, it looks good to commit, and
thanks!

Stan

Patch pending review

[Franz Sirl]

Hi,

this patch:

	[PATCH] Fix libobjc lib install (libobjc/4039)
	<http://gcc.gnu.org/ml/gcc-patches/2002-02/msg00043.html>

needs review.

Thanks,
Franz.

Re: Patch pending review

[Richard Henderson]

On Tue, Jan 01, 2002 at 09:29:26PM +0000, Joseph S. Myers wrote:
> http://gcc.gnu.org/ml/gcc-patches/2001-12/msg02508.html
> (move arch-specific headers from gcc/ginclude to gcc/config)

Ok.


r~

Patch pending review

[Joseph S. Myers]

I have the following patch pending review:

http://gcc.gnu.org/ml/gcc-patches/2001-12/msg02508.html
(move arch-specific headers from gcc/ginclude to gcc/config)

This patch is part of cleaning up currently undocumented internals
interfaces as part of improving the internals manual (starting with
creating proper systematic documentation for the source tree structure and
build system).

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

Re: Patch pending review

[Toon Moene]

"Joseph S. Myers" wrote:

> The Fortran manual part of
> 
> http://gcc.gnu.org/ml/gcc-patches/2001-10/msg00714.html
> 
> is pending review, for both the mainline and the 3.0 branch.

Geoff Keating already approved this - its OK with me too, thanks !

[ I probably missed it because I was on vacation at that time - sorry ]

-- 
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
Join GNU Fortran 95: http://g95.sourceforge.net/ (under construction)

Re: Patch pending review

[Geoff Keating]

"Joseph S. Myers" <jsm28@cam.ac.uk> writes:

> The Fortran manual part of
> 
> http://gcc.gnu.org/ml/gcc-patches/2001-10/msg00714.html
> 
> is pending review, for both the mainline and the 3.0 branch.

This is OK for both (subject to the current state of the 3.0 branch).

-- 
- Geoffrey Keating <geoffk@geoffk.org>

Patch pending review

[Joseph S. Myers]

The Fortran manual part of

http://gcc.gnu.org/ml/gcc-patches/2001-10/msg00714.html

is pending review, for both the mainline and the 3.0 branch.

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

Patch pending review

[Joseph S. Myers]

My patch to remove some obsolete files

http://gcc.gnu.org/ml/gcc-patches/2001-05/msg01529.html

still needs review of the unusedness of "listing" on mainline and branch
and "dostage2" and "dostage3" on mainline.

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

Re: Patch pending review

[Franz Sirl]

On Tuesday 12 December 2000 22:33, Geoff Keating wrote:
> > From: Franz Sirl <Franz.Sirl-kernel@lauterbach.com>
> > Date: Tue, 12 Dec 2000 21:51:06 +0100
> > Cc: gcc-patches@gcc.gnu.org
> >
> > On Tuesday 12 December 2000 20:46, Geoff Keating wrote:
> > > Franz Sirl <Franz.Sirl-kernel@lauterbach.com> writes:
> > Well, I guess it still may fail and I strongly believe
> > insn_current_reference_address() should be changed too. The code in
> > question as I understand it
> >
> >    if (INSN_SHUID (branch) < INSN_SHUID (dest))
> >     {
> >       /* Forward branch. */
> >       return (insn_last_address + insn_lengths[seq_uid]
> >               - align_fuzz (seq, dest, length_unit_log, ~0));
> >     }
> >   else
> >     {
> >       /* Backward branch. */
> >       return (insn_current_address
> >               + align_fuzz (dest, seq, length_unit_log, ~0));
> >     }
> >
> > tries to provide a threshold so we don't enter an endless loop trying to
> > shorten the branches. But this will only work as expected if
> > insn_last_address < insn_current_address. But the branch shortening can
> > happily end up with insn_last_address >= insn_current_address and thus
> > the branch maybe miscalculated depending on the compiled code, cause the
> > calculated forward branch distance is too small then.
>
> I think that the current code is correct.
>
> The first thing you should know is that for labels after the current
> insn, the address of the label hasn't been updated since the last
> pass.  So it's correct to use insn_last_address for such computations.
> By comparison, labels before the current insn _have_ been updated, so
> it's OK to use insn_current_address.

Hmm, this contradicts my experience during debugging IIRC, what happens if 
the forward branch distance gets longer during this pass?

> The next thing to know is that it seems to assume that forward
> branches are relative to the end of the branch, but backward branches
> are relative to the start of the branch.  I guess this is no worse
> than the alternatives.

Hmm.

> > If my original solution seems to strict, I would suggest this variation,
> > which should accomplish the intention:
> >
> >    if (INSN_SHUID (branch) < INSN_SHUID (dest))
> >     {
> >       /* Forward branch. */
> >       return (insn_current_address - insn_default_length (branch)
> >               - align_fuzz (seq, dest, length_unit_log, ~0));
> >     }
> >   else
> >     {
> >       /* Backward branch. */
> >       return (insn_current_address
> >               + align_fuzz (dest, seq, length_unit_log, ~0));
> >     }
> >
> > This should provide the threshold in a reliable manner due to the
> > subtraction of insn_default_length(branch).
>
> This is excessively conservative.

I don't think so, during the last pass the pessimization should be minimal. 
Well, lets see what your testing reveals.

Franz.

Re: Patch pending review

[Geoff Keating]

> From: Franz Sirl <Franz.Sirl-kernel@lauterbach.com>
> Date: Tue, 12 Dec 2000 21:51:06 +0100
> Cc: gcc-patches@gcc.gnu.org
> 
> On Tuesday 12 December 2000 20:46, Geoff Keating wrote:
> > Franz Sirl <Franz.Sirl-kernel@lauterbach.com> writes:
> > > Hi,
> > >
> > > this patch
> > >
> > > < http://gcc.gnu.org/ml/gcc-patches/2000-11/msg00202.html >
> > >
> > > hasn't been reviewed yet. It works fine and enables successful RTL
> > > checking bootstraps on powerpc-linux-gnu.
> >
> > Thank you!
> >
> > I was trying to work out why my patch, which changed it to:
> >
> > ;; Length (in bytes).
> > (define_attr "length" ""
> >   (if_then_else (eq_attr "type" "branch")
> >               (if_then_else (and (ge (minus (pc) (match_dup 0))
> >                                       (const_int -32768))
> >                                  (lt (minus (pc) (match_dup 0))
> >                                       (const_int 32764)))
> >                              (const_int 4)
> >                              (const_int 8))
> >
> > wasn't working.
> >
> > I believe the correct change is to make it
> >
> > ;; Length (in bytes).
> > ; According to insn_current_reference_address, '(pc)' in the following
> > means: ;   for a forward branch, the reference address is the end address
> > of ;   the branch as known from the previous branch shortening pass,
> > ;   minus a value to account for possible size increase due to
> > ;   alignment.  For a backward branch, it is the start address of the
> > ;   branch as known from the current pass, plus a value to account for
> > ;   possible size increase due to alignment.
> > ; so for forward branches, we need to subtract the size of the branch
> > ; (which will be 4 if the branch is being shortened) from (pc), to get
> > ; the start of the branch from which the branch offset will be
> > ; relative.  This is accomplished by using 32764 rather than 32768 below.
> > (define_attr "length" ""
> >   (if_then_else (eq_attr "type" "branch")
> >               (if_then_else (and (ge (minus (match_dup 0) (pc))
> >                                       (const_int -32768))
> >                                  (lt (minus (match_dup 0) (pc))
> >                                       (const_int 32764)))
> >                              (const_int 4)
> >                              (const_int 8))
> >
> > and drop the generic change.  (The generic change would break many
> > other ports.)  I'll test this and see how it goes.
> 
> Well, I guess it still may fail and I strongly believe 
> insn_current_reference_address() should be changed too. The code in question 
> as I understand it
> 
>    if (INSN_SHUID (branch) < INSN_SHUID (dest))
>     {
>       /* Forward branch. */
>       return (insn_last_address + insn_lengths[seq_uid]
>               - align_fuzz (seq, dest, length_unit_log, ~0));
>     }
>   else
>     {
>       /* Backward branch. */
>       return (insn_current_address
>               + align_fuzz (dest, seq, length_unit_log, ~0));
>     }
> 
> tries to provide a threshold so we don't enter an endless loop trying to 
> shorten the branches. But this will only work as expected if 
> insn_last_address < insn_current_address. But the branch shortening can 
> happily end up with insn_last_address >= insn_current_address and thus the 
> branch maybe miscalculated depending on the compiled code, cause the 
> calculated forward branch distance is too small then.

I think that the current code is correct.

The first thing you should know is that for labels after the current
insn, the address of the label hasn't been updated since the last
pass.  So it's correct to use insn_last_address for such computations.
By comparison, labels before the current insn _have_ been updated, so
it's OK to use insn_current_address.

The next thing to know is that it seems to assume that forward
branches are relative to the end of the branch, but backward branches
are relative to the start of the branch.  I guess this is no worse
than the alternatives.

> If my original solution seems to strict, I would suggest this variation, 
> which should accomplish the intention:
> 
>    if (INSN_SHUID (branch) < INSN_SHUID (dest))
>     {
>       /* Forward branch. */
>       return (insn_current_address - insn_default_length (branch)
>               - align_fuzz (seq, dest, length_unit_log, ~0));
>     }
>   else
>     {
>       /* Backward branch. */
>       return (insn_current_address
>               + align_fuzz (dest, seq, length_unit_log, ~0));
>     }
> 
> This should provide the threshold in a reliable manner due to the
> subtraction of insn_default_length(branch).

This is excessively conservative.

-- 
- Geoffrey Keating <geoffk@geoffk.org>

Re: Patch pending review

[Franz Sirl]

On Tuesday 12 December 2000 20:46, Geoff Keating wrote:
> Franz Sirl <Franz.Sirl-kernel@lauterbach.com> writes:
> > Hi,
> >
> > this patch
> >
> > < http://gcc.gnu.org/ml/gcc-patches/2000-11/msg00202.html >
> >
> > hasn't been reviewed yet. It works fine and enables successful RTL
> > checking bootstraps on powerpc-linux-gnu.
>
> Thank you!
>
> I was trying to work out why my patch, which changed it to:
>
> ;; Length (in bytes).
> (define_attr "length" ""
>   (if_then_else (eq_attr "type" "branch")
>               (if_then_else (and (ge (minus (pc) (match_dup 0))
>                                       (const_int -32768))
>                                  (lt (minus (pc) (match_dup 0))
>                                       (const_int 32764)))
>                              (const_int 4)
>                              (const_int 8))
>
> wasn't working.
>
> I believe the correct change is to make it
>
> ;; Length (in bytes).
> ; According to insn_current_reference_address, '(pc)' in the following
> means: ;   for a forward branch, the reference address is the end address
> of ;   the branch as known from the previous branch shortening pass,
> ;   minus a value to account for possible size increase due to
> ;   alignment.  For a backward branch, it is the start address of the
> ;   branch as known from the current pass, plus a value to account for
> ;   possible size increase due to alignment.
> ; so for forward branches, we need to subtract the size of the branch
> ; (which will be 4 if the branch is being shortened) from (pc), to get
> ; the start of the branch from which the branch offset will be
> ; relative.  This is accomplished by using 32764 rather than 32768 below.
> (define_attr "length" ""
>   (if_then_else (eq_attr "type" "branch")
>               (if_then_else (and (ge (minus (match_dup 0) (pc))
>                                       (const_int -32768))
>                                  (lt (minus (match_dup 0) (pc))
>                                       (const_int 32764)))
>                              (const_int 4)
>                              (const_int 8))
>
> and drop the generic change.  (The generic change would break many
> other ports.)  I'll test this and see how it goes.

Well, I guess it still may fail and I strongly believe 
insn_current_reference_address() should be changed too. The code in question 
as I understand it

   if (INSN_SHUID (branch) < INSN_SHUID (dest))
    {
      /* Forward branch. */
      return (insn_last_address + insn_lengths[seq_uid]
              - align_fuzz (seq, dest, length_unit_log, ~0));
    }
  else
    {
      /* Backward branch. */
      return (insn_current_address
              + align_fuzz (dest, seq, length_unit_log, ~0));
    }

tries to provide a threshold so we don't enter an endless loop trying to 
shorten the branches. But this will only work as expected if 
insn_last_address < insn_current_address. But the branch shortening can 
happily end up with insn_last_address >= insn_current_address and thus the 
branch maybe miscalculated depending on the compiled code, cause the 
calculated forward branch distance is too small then.

If my original solution seems to strict, I would suggest this variation, 
which should accomplish the intention:

   if (INSN_SHUID (branch) < INSN_SHUID (dest))
    {
      /* Forward branch. */
      return (insn_current_address - insn_default_length (branch)
              - align_fuzz (seq, dest, length_unit_log, ~0));
    }
  else
    {
      /* Backward branch. */
      return (insn_current_address
              + align_fuzz (dest, seq, length_unit_log, ~0));
    }

This should provide the threshold in a reliable manner due to the subtraction 
of insn_default_length(branch).

Franz.

Re: Patch pending review

[Geoff Keating]

Franz Sirl <Franz.Sirl-kernel@lauterbach.com> writes:

> Hi,
> 
> this patch
> 
> < http://gcc.gnu.org/ml/gcc-patches/2000-11/msg00202.html >
> 
> hasn't been reviewed yet. It works fine and enables successful RTL checking
> bootstraps on powerpc-linux-gnu.

Thank you!

I was trying to work out why my patch, which changed it to:

;; Length (in bytes).
(define_attr "length" ""
  (if_then_else (eq_attr "type" "branch")
              (if_then_else (and (ge (minus (pc) (match_dup 0))
                                      (const_int -32768))
                                 (lt (minus (pc) (match_dup 0))
                                      (const_int 32764)))
                             (const_int 4)
                             (const_int 8))

wasn't working.

I believe the correct change is to make it

;; Length (in bytes).
; According to insn_current_reference_address, '(pc)' in the following means:
;   for a forward branch, the reference address is the end address of
;   the branch as known from the previous branch shortening pass,
;   minus a value to account for possible size increase due to
;   alignment.  For a backward branch, it is the start address of the
;   branch as known from the current pass, plus a value to account for
;   possible size increase due to alignment.
; so for forward branches, we need to subtract the size of the branch
; (which will be 4 if the branch is being shortened) from (pc), to get
; the start of the branch from which the branch offset will be
; relative.  This is accomplished by using 32764 rather than 32768 below.
(define_attr "length" ""
  (if_then_else (eq_attr "type" "branch")
              (if_then_else (and (ge (minus (match_dup 0) (pc))
                                      (const_int -32768))
                                 (lt (minus (match_dup 0) (pc))
                                      (const_int 32764)))
                             (const_int 4)
                             (const_int 8))

and drop the generic change.  (The generic change would break many
other ports.)  I'll test this and see how it goes.

-- 
- Geoffrey Keating <geoffk@geoffk.org>

Patch pending review

[Franz Sirl]

Hi,

this patch

< http://gcc.gnu.org/ml/gcc-patches/2000-11/msg00202.html >

hasn't been reviewed yet. It works fine and enables successful RTL checking
bootstraps on powerpc-linux-gnu.

Thanks,
Franz.