Patch: make mapped locations the default

Patch: make mapped locations the default

[Tom Tromey]

This patch changes gcc/configure.ac to make mapped locations the
default.

AIUI, all of the front ends have been updated.

Bootstrapped and regtested for c,c++,objc,fortran,treelang,java (but
not ada) on x86 FC-6.

Ok?

Tom

ChangeLog:
2007-10-25  Tom Tromey  <tromey@redhat.com>

	* configure: Rebuilt.
	* configure.ac (--enable-mapped-location): Default to 'yes'.

Index: configure.ac
===================================================================
--- configure.ac	(revision 129492)
+++ configure.ac	(working copy)
@@ -525,7 +525,7 @@
 
 AC_ARG_ENABLE(mapped-location,
 [  --enable-mapped-location   location_t is fileline integer cookie],,
-enable_mapped_location=no)
+enable_mapped_location=yes)
 
 if test "$enable_mapped_location" = yes ; then
   AC_DEFINE(USE_MAPPED_LOCATION, 1,

Re: Patch: make mapped locations the default

[Eric Botcazou]

> Bootstrapped and regtested for c,c++,objc,fortran,treelang,java (but
> not ada) on x86 FC-6.

It would be nice to test Ada as well for such a big change.

-- 
Eric Botcazou

Re: Patch: make mapped locations the default

[Tom Tromey]

>>>>> "Eric" == Eric Botcazou <ebotcazou@adacore.com> writes:

>> Bootstrapped and regtested for c,c++,objc,fortran,treelang,java (but
>> not ada) on x86 FC-6.

Eric> It would be nice to test Ada as well for such a big change.

Ok.  I'll try to do this next week sometime.

Tom

Re: Patch: make mapped locations the default

[Tom Tromey]

>>>>> "Eric" == Eric Botcazou <ebotcazou@adacore.com> writes:

>> Bootstrapped and regtested for c,c++,objc,fortran,treelang,java (but
>> not ada) on x86 FC-6.

Eric> It would be nice to test Ada as well for such a big change.

I did this and it passed.

Tom

Re: Patch: make mapped locations the default

[Eric Botcazou]

> I did this and it passed.

Neat. :-)  Thanks for the testing.

-- 
Eric Botcazou

Re: Patch: make mapped locations the default

[Mark Mitchell]

Eric Botcazou wrote:
>> I did this and it passed.
> 
> Neat. :-)  Thanks for the testing.

So, Tom, where are we now, from a technical perspective?  To the best of
your knowledge, could we switch to mapped locations now without
suffering any ill effects?

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Patch: make mapped locations the default

[Tom Tromey]

>>>>> "Mark" == Mark Mitchell <mark@codesourcery.com> writes:

Mark> Eric Botcazou wrote:
>>> I did this and it passed.
>> 
>> Neat. :-)  Thanks for the testing.

Mark> So, Tom, where are we now, from a technical perspective?  To the best of
Mark> your knowledge, could we switch to mapped locations now without
Mark> suffering any ill effects?

Yes, there is one ill effect, which is that users will get incorrect
column numbers in error messages.

The simplest way to fix this is to disable column number output
However, it might be worth trying it out and seeing whether the bugs
can be fixed in a timely way.  I would recommend that we disable
column output after branching, if the bugs aren't fixed.

I think flipping the default is the best plan (even though, eww, it
makes life hard for the incremental compiler), due to the possibility
of regressions if we don't change it.  Plus which, as mentioned
before, it is a win in memory use (though I have not tried to measure
this).

Tom

Re: Patch: make mapped locations the default

[Mark Mitchell]

Tom Tromey wrote:
>>>>>> "Mark" == Mark Mitchell <mark@codesourcery.com> writes:
> 
> Mark> Eric Botcazou wrote:
>>>> I did this and it passed.
>>> Neat. :-)  Thanks for the testing.
> 
> Mark> So, Tom, where are we now, from a technical perspective?  To the best of
> Mark> your knowledge, could we switch to mapped locations now without
> Mark> suffering any ill effects?
> 
> Yes, there is one ill effect, which is that users will get incorrect
> column numbers in error messages.

Since we don't presently emit column numbers we can, as you say, just
disable column number output.

Since this project has been around for so long, and was on the 4.3 plan,
my inclination is as follows:

1. Disable column number output.
2. Change the default.

Then, when column numbers are made to work, you can turn them on.  (I
don't see any reason to have them on -- but broken -- while we fix the
bugs; it's easy enough for people who want to fix the problems to turn
them on locally.)

Are there any objections (from you or others) to the plan above?  If
there are no objections within 48 hours, please proceed with that plan.

> of regressions if we don't change it.  Plus which, as mentioned
> before, it is a win in memory use (though I have not tried to measure
> this).

That would be a good thing to do, since it is the primary motivation for
the change, as I understand it.  IIUC, you're essentially replacing a {
file, line } pair with a single integer.

I look forward to finally checking this off the list -- as I'm sure you
do as well!

Thanks for your patience,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Patch: make mapped locations the default

[Tom Tromey]

>>>>> "Mark" == Mark Mitchell <mark@codesourcery.com> writes:

Mark> Since this project has been around for so long, and was on the
Mark> 4.3 plan, my inclination is as follows:

Mark> 1. Disable column number output.
Mark> 2. Change the default.

Mark> Then, when column numbers are made to work, you can turn them on.  (I
Mark> don't see any reason to have them on -- but broken -- while we fix the
Mark> bugs; it's easy enough for people who want to fix the problems to turn
Mark> them on locally.)

The problem with this, and the reason I suggested the opposite course,
is that I think the history of mapped locations shows that if we
disable this now, the bugs will simply never be fixed.

Still, I'll proceed on schedule unless someone else objects.

Tom

Re: Patch: make mapped locations the default

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

>>>>> "Mark" == Mark Mitchell <mark@codesourcery.com> writes:
Mark> Since this project has been around for so long, and was on the
Mark> 4.3 plan, my inclination is as follows:
Mark> 1. Disable column number output.
Mark> 2. Change the default.

Tom> Still, I'll proceed on schedule unless someone else objects.

Here's the updated patch.

This works by flipping the default for -fshow-column.  We still get
column numbers from cpp, because gcc only changes libcpp's setting
when the command-line option is seen.

With this approach, developers can easily enable column number output
with -fshow-column.

Bootstrapped and regtested on x86 FC-6.  Ok?

Tom

ChangeLog:
2007-11-07  Tom Tromey  <tromey@redhat.com>

	* common.opt (fshow-column): Default to 0.
	* configure: Rebuilt.
	* configure.ac (--enable-mapped-location): Default to 'yes'.

Index: configure.ac
===================================================================
--- configure.ac	(revision 129968)
+++ configure.ac	(working copy)
@@ -525,7 +525,7 @@
 
 AC_ARG_ENABLE(mapped-location,
 [  --enable-mapped-location   location_t is fileline integer cookie],,
-enable_mapped_location=no)
+enable_mapped_location=yes)
 
 if test "$enable_mapped_location" = yes ; then
   AC_DEFINE(USE_MAPPED_LOCATION, 1,
Index: common.opt
===================================================================
--- common.opt	(revision 129968)
+++ common.opt	(working copy)
@@ -926,7 +926,7 @@
 Eliminate redundant sign extensions using LCM.
 
 fshow-column
-Common C ObjC C++ ObjC++ Report Var(flag_show_column) Init(1)
+Common C ObjC C++ ObjC++ Report Var(flag_show_column) Init(0)
 Show column numbers in diagnostics, when available.  Default on
 
 fsignaling-nans

Re: Patch: make mapped locations the default

[Manuel López-Ibáñez]

On 08/11/2007, Tom Tromey <tromey@redhat.com> wrote:
>
> With this approach, developers can easily enable column number output
> with -fshow-column.
>

Just an idea. Why not enable it while the branch is experimental and
disable it for release? Alternatively, we could just disable it for
now and enable it as soon as 4.3 is branched.

Cheers,

Manuel.

Re: Patch: make mapped locations the default

[Tom Tromey]

>>>>> "Manuel" == Manuel López-Ibáñez <lopezibanez@gmail.com> writes:


>> With this approach, developers can easily enable column number output
>> with -fshow-column.

Manuel> Just an idea. Why not enable it while the branch is experimental and
Manuel> disable it for release? Alternatively, we could just disable it for
Manuel> now and enable it as soon as 4.3 is branched.

I initially proposed leaving column numbers on by default until the
release.  This patch implements what Mark asked for instead of that.

Tom

Re: Patch: make mapped locations the default

[Andreas Schwab]

Tom Tromey <tromey@redhat.com> writes:

> Index: common.opt
> ===================================================================
> --- common.opt	(revision 129968)
> +++ common.opt	(working copy)
> @@ -926,7 +926,7 @@
>  Eliminate redundant sign extensions using LCM.
>  
>  fshow-column
> -Common C ObjC C++ ObjC++ Report Var(flag_show_column) Init(1)
> +Common C ObjC C++ ObjC++ Report Var(flag_show_column) Init(0)
>  Show column numbers in diagnostics, when available.  Default on
                                                        ^^^^^^^^^^
No longer true.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Patch: make mapped locations the default

[Tom Tromey]

>>>>> "Andreas" == Andreas Schwab <schwab@suse.de> writes:

[..]
Andreas> No longer true.

Oops.  Thanks for catching that.  Here's the fixed patch.

Tom

ChangeLog:
2007-11-08  Tom Tromey  <tromey@redhat.com>

	* common.opt (fshow-column): Default to 0.
	* configure: Rebuilt.
	* configure.ac (--enable-mapped-location): Default to 'yes'.

Index: configure.ac
===================================================================
--- configure.ac	(revision 129968)
+++ configure.ac	(working copy)
@@ -525,7 +525,7 @@
 
 AC_ARG_ENABLE(mapped-location,
 [  --enable-mapped-location   location_t is fileline integer cookie],,
-enable_mapped_location=no)
+enable_mapped_location=yes)
 
 if test "$enable_mapped_location" = yes ; then
   AC_DEFINE(USE_MAPPED_LOCATION, 1,
Index: common.opt
===================================================================
--- common.opt	(revision 129968)
+++ common.opt	(working copy)
@@ -24,6 +24,7 @@
 
 -help
 Common
+
 Display this information
 
 -help=
@@ -926,8 +927,8 @@
 Eliminate redundant sign extensions using LCM.
 
 fshow-column
-Common C ObjC C++ ObjC++ Report Var(flag_show_column) Init(1)
-Show column numbers in diagnostics, when available.  Default on
+Common C ObjC C++ ObjC++ Report Var(flag_show_column) Init(0)
+Show column numbers in diagnostics, when available.  Default off
 
 fsignaling-nans
 Common Report Var(flag_signaling_nans) Optimization

Re: Patch: make mapped locations the default

[Mark Mitchell]

Tom Tromey wrote:

> ChangeLog:
> 2007-11-08  Tom Tromey  <tromey@redhat.com>
> 
> 	* common.opt (fshow-column): Default to 0.
> 	* configure: Rebuilt.
> 	* configure.ac (--enable-mapped-location): Default to 'yes'.

OK.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Patch: make mapped locations the default

[Dirk Mueller]

On Thursday 08 November 2007, Mark Mitchell wrote:

> > 2007-11-08  Tom Tromey  <tromey@redhat.com>
> >
> > 	* common.opt (fshow-column): Default to 0.
> > 	* configure: Rebuilt.
> > 	* configure.ac (--enable-mapped-location): Default to 'yes'.

The patch seems to break --enable-werror bootstrap here (i586) with an 
array-underflow warning in reg-stack.c. It is not immediately obvious to me 
why that happens. Am I really the only one seeing this?

As it might cause problems during regtesting other patches, can we perhaps 
back that change out until the underlying bug is fixed?

Thanks,
Dirk

Re: Patch: make mapped locations the default

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> This patch changes gcc/configure.ac to make mapped locations the
Tom> default.

After playing with this a bit, I think we should disable column number
output.  It is very easy to find places where the C and C++ front
ends, at least, emit bogus column information.

Also, fixing this is not entirely simple.  Well, for decls it is
reasonably simple... I have a patch for the C FE that I'll send soon;
it fixes most cases.

However, for expressions it does not look very easy.  I think it would
be risky (not to mention ugly) to change build* to set the default
location of expressions from input_location.  Even if we did this we'd
have to make sure input_location is set properly -- which it generally
is not.

Likewise, changing the API of build* or fold_build* is fairly involved
(if not this would be my preferred approach, as I think it is the best
API design -- in my view the location ought to be set in the
constructor and then never changed).

Moving out another layer, I suppose we could fix build_unary_op and
friends.  That is still a reasonably large number of call sites.  It
doesn't seem easy to apply the fix in the callers of these functions,
due to default_conversion.

Any comments?  Disagreements with my analysis?  Not that I usually
have to ask about disagreements ;)

Note that with mapped locations there is no space penalty to assigning
a location to every expression.

Tom