Re: [rfc][00/37] Eliminate builtin_type_ macros

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Ulrich Weigand]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

Joel Brobecker wrote:

>OK, I ended up checking in both patches (there was simply too much
>confusion on the second one, so it was simpler if I just change
>the use of builtin_type_int into builtin_type_int32).  Hopefully,
>this should clear the way for you to commit your series of patches!

(Hmm, it seems my first reply didn't make it to the list; I'll try again
with the patch compressed to make it shorter ...)

I've now checked in the remaining parts of the patch series.
The cumulative patch as checked-in is appended.



Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

[-- Attachment #2: builtin-type-all.diff.bz2 --]
[-- Type: application/octet-stream, Size: 50079 bytes --]

[rfc][00/37] Eliminate builtin_type_ macros

[uweigand]

Hello,

one of the last remaining "implicit" uses of current_gdbarch is the
builtin_type_ macros.  This patch set completely removes those macros.

I've attempted to not simply replace those types with explict uses of
current_gdbarch, but instead choose the proper architecture to use:

- I've added a gdbarch pointer to struct expression, and used this
  per-expression arch throughout expression parsing and evaluation.
- I've converted some types (like builtin_void_type) back to be
  platform-neutral, so they can be freely used throughout GDB.
- In many cases, an existing per-frame or per-objfile arch was available
  (or could be made available) and should be used.
- Target-specific code should use target_gdbarch.
- In a very small number of case, current_gdbarch remains for now.

Each of the following patches contains a more specific explanation.

Combination of all 37 patches tested on amd64-linux, s390-ibm-linux,
s390x-ibm-linux, powerpc-linux, powerpc64-linux, and spu-elf with no
regressions.  Also, compile-tested with --enable-targets=all (each 
patch by itself).

I'd be interested in particular in feedback from the Ada maintainers
on the Ada-related changes in this patch-set.

What do you think?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Mark Kettenis]

> Date: Sun, 31 Aug 2008 19:50:45 +0200
> From: uweigand@de.ibm.com
> 
> Hello,
> 
> one of the last remaining "implicit" uses of current_gdbarch is the
> builtin_type_ macros.  This patch set completely removes those macros.
> 
> I've attempted to not simply replace those types with explict uses of
> current_gdbarch, but instead choose the proper architecture to use:
> 
> - I've added a gdbarch pointer to struct expression, and used this
>   per-expression arch throughout expression parsing and evaluation.
> - I've converted some types (like builtin_void_type) back to be
>   platform-neutral, so they can be freely used throughout GDB.
> - In many cases, an existing per-frame or per-objfile arch was available
>   (or could be made available) and should be used.
> - Target-specific code should use target_gdbarch.
> - In a very small number of case, current_gdbarch remains for now.
> 
> Each of the following patches contains a more specific explanation.
> 
> Combination of all 37 patches tested on amd64-linux, s390-ibm-linux,
> s390x-ibm-linux, powerpc-linux, powerpc64-linux, and spu-elf with no
> regressions.  Also, compile-tested with --enable-targets=all (each 
> patch by itself).
> 
> I'd be interested in particular in feedback from the Ada maintainers
> on the Ada-related changes in this patch-set.
> 
> What do you think?

I've probably had one piwo too many at this point, but can we please
stop this Linux [x/zillion] crap?  You can't seriously pretend there
are really 37 independent diffs that people would want to review
and/or test can you?

Re: [rfc][00/37] Eliminate builtin_type_ macros

[David Miller]

From: Mark Kettenis <mark.kettenis@xs4all.nl>
Date: Mon, 1 Sep 2008 00:18:16 +0200 (CEST)

> I've probably had one piwo too many at this point, but can we please
> stop this Linux [x/zillion] crap?  You can't seriously pretend there
> are really 37 independent diffs that people would want to review
> and/or test can you?

Why is it crap to split up a large harder to review (and bisect)
change into bite size (easier to review and later bisect) independant
pieces?  And why is it purely labelled as "a Linux thing" to split up
a large patch this way?  I've seen this submission technique used in
many other projects.

If a reviewer doesn't have time to review the whole bit, they can
choose to look at one a few or even just one of them, when it is
split up like this.

What if later there is a regression and someone tries to bisect
through this change?  If we have just one huge one it's harder to
figure out which of the 37 parts caused the problem, whereas if
we could bisect to one of these 37 pieces, we'd know precisely which
part added the regression.

That is just smart development as far as I can see.

The response I am seeing seems like a negative knee jerk reaction to
me.  I post sets of 50 patches at a time frequently for some of my
work, and it eases the reviewer burdon tremendously.

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Ulrich Weigand]

Mark Kettenis wrote:

> I've probably had one piwo too many at this point, but can we please
> stop this Linux [x/zillion] crap?  You can't seriously pretend there
> are really 37 independent diffs that people would want to review
> and/or test can you?

Actually, the patches *do* touch mostly independent areas of GDB,
and I'd expect different maintainers to want to review only some
of them.  I've spent some effort to try to separate out functional
changes, in the hope of making review simpler ...

As to *testing*, I agree that having to apply 37 patches in sequence
is a pain, which is why I sent -in addition to the broken-out series-
a single cumulative patch as well.

In the end, this is simply a large set of changes (the cumulative patch
is 8000 lines, the broken-out patches total 10000 lines) spread out
across many parts of GDB (the patch set touches 97 files) -- if you have
suggestions how to present a change like this in a way that's easier to
review, those would certainly be welcome.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Mark Kettenis]

> Date: Mon, 1 Sep 2008 20:56:41 +0200 (CEST)
> From: "Ulrich Weigand" <uweigand@de.ibm.com>
> 
> Mark Kettenis wrote:
> 
> > I've probably had one piwo too many at this point, but can we please
> > stop this Linux [x/zillion] crap?  You can't seriously pretend there
> > are really 37 independent diffs that people would want to review
> > and/or test can you?
> 
> Actually, the patches *do* touch mostly independent areas of GDB,
> and I'd expect different maintainers to want to review only some
> of them.  I've spent some effort to try to separate out functional
> changes, in the hope of making review simpler ...

Let me say that even if I think it doesn't really help, I apreciate
the effort.

> As to *testing*, I agree that having to apply 37 patches in sequence
> is a pain, which is why I sent -in addition to the broken-out series-
> a single cumulative patch as well.

Yes, that was a good thing to do.  I apologize for sending the message
I sent yesterday evening before reading all my mail.

> In the end, this is simply a large set of changes (the cumulative patch
> is 8000 lines, the broken-out patches total 10000 lines) spread out
> across many parts of GDB (the patch set touches 97 files) -- if you have
> suggestions how to present a change like this in a way that's easier to
> review, those would certainly be welcome.

I don't think there is much you can do about it.  A large set of
fairly mechanical changes is simply a large set of mechanical changes.
It's probably good if people have a look at part of the diff, but in
the end we'll just have to trust that the job was done properly and
that it gets committed (preferably after people have tested it).

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Daniel Jacobowitz]

On Tue, Sep 02, 2008 at 12:20:44PM +0200, Mark Kettenis wrote:
> I don't think there is much you can do about it.  A large set of
> fairly mechanical changes is simply a large set of mechanical changes.
> It's probably good if people have a look at part of the diff, but in
> the end we'll just have to trust that the job was done properly and
> that it gets committed (preferably after people have tested it).

I agree.  FWIW, I found this helpful - there was one problem in
particular where I wanted to see Ulrich's solution, and I was able to
find it easily :-)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Ulrich Weigand]

Mark Kettenis wrote:

> > As to *testing*, I agree that having to apply 37 patches in sequence
> > is a pain, which is why I sent -in addition to the broken-out series-
> > a single cumulative patch as well.
> 
> Yes, that was a good thing to do.  I apologize for sending the message
> I sent yesterday evening before reading all my mail.

No problem -- I appreciate the feedback.

> > In the end, this is simply a large set of changes (the cumulative patch
> > is 8000 lines, the broken-out patches total 10000 lines) spread out
> > across many parts of GDB (the patch set touches 97 files) -- if you have
> > suggestions how to present a change like this in a way that's easier to
> > review, those would certainly be welcome.
> 
> I don't think there is much you can do about it.  A large set of
> fairly mechanical changes is simply a large set of mechanical changes.
> It's probably good if people have a look at part of the diff, but in
> the end we'll just have to trust that the job was done properly and
> that it gets committed (preferably after people have tested it).

The thing is, parts of the patch set (e.g. the -tdep.c changes) are
indeed completely mechanical changes.  However, in other places I am
making definite design choices (e.g. should an expression really be
something architecture-neutral, or explicitly platform-specific?).

One important reason for splitting the patch set up is in fact to
avoid such design choices being swamped and overlooked within a
single large, mostly mechanical patch ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Daniel Jacobowitz]

On Sun, Aug 31, 2008 at 07:50:45PM +0200, Ulrich Weigand wrote:
> What do you think?

All the others I didn't respond to look OK to me.  I'm mildly
concerned about the contortions you had to go through; it becomes hard
to get at the architecture in new places.  But we'll have to see how
that works out in practice.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> On Sun, Aug 31, 2008 at 07:50:45PM +0200, Ulrich Weigand wrote:
> > What do you think?
> 
> All the others I didn't respond to look OK to me.

Thanks for looking over them!

> I'm mildly
> concerned about the contortions you had to go through; it becomes hard
> to get at the architecture in new places.  But we'll have to see how
> that works out in practice.

To some extent, there is a choice of direction here.  Today, we have a
number of low-level routines that have implicit dependency on the 
"current" architecture.  To fix this, there are really two directions:

- Re-define the lower-level parts to be architecture-independent, and
  push architecture-dependent semantics up.
or
- Make lower-level entities *explicitly* architecture-specific

For example, should a "value" be platform-neutral or platform-specific?
If value is platform-neutral, we'll need to take explicit care at the
locations where values get in contact with platform-specific code, e.g.
when loading/storing from target memory.  If value is platform-specific,
it should probably carry an explicit gdbarch pointer.  Then we can -in
a multi-arch setting- get into the situation what to do with values of
a different architecture than the current frame ...

With this patch-set, I've made "expression" explicitly platform-specific,
but tried to keep "value" and "type" more platform-independent.  I guess
we'll have to see if this is right choice.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Daniel Jacobowitz]

On Wed, Sep 03, 2008 at 12:01:55AM +0200, Ulrich Weigand wrote:
> With this patch-set, I've made "expression" explicitly platform-specific,
> but tried to keep "value" and "type" more platform-independent.  I guess
> we'll have to see if this is right choice.

It makes sense to me, after some staring at it.  I think the right
approach is to make the common cases straightforward, and reject some
of the hardest cases - for instance, I don't know that a single
expression calling functions belonging to two different architectures
will ever matter, in practice.

[Watch the Cell debugger prove me wrong...]

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Joel Brobecker]

> What do you think?

I think I have now reviewed all 37 patches and sent all my comments.
I would like to test this patch against our testsuite, but unfortunately
it will be a few more days at least before I can do so. If I get to it
before the patch is checked in, I will send the results...

-- 
Joel

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Ulrich Weigand]

Joel Brobecker wrote:

> I think I have now reviewed all 37 patches and sent all my comments.

Many thanks for the thorough review1

> I would like to test this patch against our testsuite, but unfortunately
> it will be a few more days at least before I can do so. If I get to it
> before the patch is checked in, I will send the results...

I can wait a couple of days before checking them in, no problem ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Joel Brobecker]

> > I would like to test this patch against our testsuite, but unfortunately
> > it will be a few more days at least before I can do so. If I get to it
> > before the patch is checked in, I will send the results...
> 
> I can wait a couple of days before checking them in, no problem ...

I just finished a first round of testing. Noticed that the gnu-v3-abi
changes don't seem to apply anymore (I think because of your recent
change, actually! ;-).

I made the modifications suggested in the 19/37 patch discussion.

I still got a few regressions with our testsuite, which I will
investigate now. Can you hold the patch off a little while longer?

Cheers,
-- 
Joel

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Ulrich Weigand]

Joel Brobecker wrote:

> I just finished a first round of testing. Noticed that the gnu-v3-abi
> changes don't seem to apply anymore (I think because of your recent
> change, actually! ;-).

Yes, that's why I posted an updated version of the patch -- sorry for
the confusion:
http://sourceware.org/ml/gdb-patches/2008-09/msg00111.html

> I made the modifications suggested in the 19/37 patch discussion.
> 
> I still got a few regressions with our testsuite, which I will
> investigate now. Can you hold the patch off a little while longer?

Certainly!  Thanks for testing ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]

Hi Ulrich,

> I still got a few regressions with our testsuite, which I will
> investigate now. Can you hold the patch off a little while longer?

OK, our testsuite found 2 issues:
 
  1. Pointer arithmetics, in particular "PTR + PTR" or "PTR - PTR"
     expressions. For instance:

       (gdb) print b'address - a'address
       Argument to arithmetic operation not a number or boolean.
     
     It's worth mentioning that the problem was already present
     with pointer addition (adding two pointers doesn't necessarily
     make a lot of sense, but anyway...).

     The regression on the substraction is because we replaced the
     call to (rip'ed) value_sub by a call to value_binop, which
     doesn't support pointer differences.

     I think the semantics of pointer differences in Ada are different
     from C. It's just a number substraction.  So I just added support
     for it directly at the caller site, thus calling value_binop
     only for values that it supports. Same for addition.

  2. The second problem is just an oversight. You needed a variable
     to store the int builtin type, and unfortunately you reused
     a variable that was still in use.
     See ada-lang.c (evaluate_subexp) [OP_ATR_SIZE].

     For now, I just used builtin_type_int32. Not ideal, but should
     be large enough for the vast majority of objects we actually
     have to deal with in real life.

Two suggested patches attached...

BTW: I tested on x86-linux (DWARF & STABS) as well as on x86_64-linux
(DWARF only, obviously).

-- 
Joel

[-- Attachment #2: 01-ptrdiff.diff --]
[-- Type: text/plain, Size: 1252 bytes --]

diff -r ef6e8d4d28b4 -r c0202064b824 ada-lang.c
--- a/ada-lang.c	Tue Sep 09 10:27:10 2008 -0700
+++ b/ada-lang.c	Tue Sep 09 12:31:38 2008 -0700
@@ -9909,6 +9909,10 @@ ada_evaluate_subexp (struct type *expect
       arg2 = evaluate_subexp_with_coercion (exp, pos, noside);
       if (noside == EVAL_SKIP)
         goto nosideret;
+      if (TYPE_CODE (value_type (arg1)) == TYPE_CODE_PTR)
+        return (value_from_longest
+                 (value_type (arg1),
+                  value_as_long (arg1) + value_as_long (arg2)));
       if ((ada_is_fixed_point_type (value_type (arg1))
            || ada_is_fixed_point_type (value_type (arg2)))
           && value_type (arg1) != value_type (arg2))
@@ -9927,6 +9931,10 @@ ada_evaluate_subexp (struct type *expect
       arg2 = evaluate_subexp_with_coercion (exp, pos, noside);
       if (noside == EVAL_SKIP)
         goto nosideret;
+      if (TYPE_CODE (value_type (arg1)) == TYPE_CODE_PTR)
+        return (value_from_longest
+                 (value_type (arg1),
+                  value_as_long (arg1) - value_as_long (arg2)));
       if ((ada_is_fixed_point_type (value_type (arg1))
            || ada_is_fixed_point_type (value_type (arg2)))
           && value_type (arg1) != value_type (arg2))

[-- Attachment #3: 02-size_atr.diff --]
[-- Type: text/plain, Size: 749 bytes --]

diff -r c0202064b824 -r 61698126d402 ChangeLog.joel
diff -r c0202064b824 -r 61698126d402 ada-lang.c
--- a/ada-lang.c	Tue Sep 09 12:31:38 2008 -0700
+++ b/ada-lang.c	Tue Sep 09 12:57:26 2008 -0700
@@ -10541,11 +10541,10 @@ ada_evaluate_subexp (struct type *expect
 
       if (noside == EVAL_SKIP)
         goto nosideret;
-      type = builtin_type (exp->gdbarch)->builtin_int;
       if (noside == EVAL_AVOID_SIDE_EFFECTS)
-        return value_zero (type, not_lval);
-      else
-        return value_from_longest (type,
+        return value_zero (builtin_type_int32, not_lval);
+      else
+        return value_from_longest (builtin_type_int32,
                                    TARGET_CHAR_BIT * TYPE_LENGTH (type));
 
     case OP_ATR_VAL:

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Ulrich Weigand]

Joel Brobecker wrote:

>   1. Pointer arithmetics, in particular "PTR + PTR" or "PTR - PTR"
>      expressions. For instance:
> 
>        (gdb) print b'address - a'address
>        Argument to arithmetic operation not a number or boolean.
>      
>      It's worth mentioning that the problem was already present
>      with pointer addition (adding two pointers doesn't necessarily
>      make a lot of sense, but anyway...).
> 
>      The regression on the substraction is because we replaced the
>      call to (rip'ed) value_sub by a call to value_binop, which
>      doesn't support pointer differences.
> 
>      I think the semantics of pointer differences in Ada are different
>      from C. It's just a number substraction.  So I just added support
>      for it directly at the caller site, thus calling value_binop
>      only for values that it supports. Same for addition.

I see.  In that case, your patch would be a bugfix completely
independently of my patch set.  Do you want to commit it right away?

>   2. The second problem is just an oversight. You needed a variable
>      to store the int builtin type, and unfortunately you reused
>      a variable that was still in use.
>      See ada-lang.c (evaluate_subexp) [OP_ATR_SIZE].
> 
>      For now, I just used builtin_type_int32. Not ideal, but should
>      be large enough for the vast majority of objects we actually
>      have to deal with in real life.

Huh?  I'm not sure what base this patch is against:

> --- a/ada-lang.c	Tue Sep 09 12:31:38 2008 -0700
> +++ b/ada-lang.c	Tue Sep 09 12:57:26 2008 -0700
> @@ -10541,11 +10541,10 @@ ada_evaluate_subexp (struct type *expect
>  
>        if (noside == EVAL_SKIP)
>          goto nosideret;
> -      type = builtin_type (exp->gdbarch)->builtin_int;
>        if (noside == EVAL_AVOID_SIDE_EFFECTS)
> -        return value_zero (type, not_lval);
> -      else
> -        return value_from_longest (type,
> +        return value_zero (builtin_type_int32, not_lval);
> +      else
> +        return value_from_longest (builtin_type_int32,
>                                     TARGET_CHAR_BIT * TYPE_LENGTH (type));
>  
>      case OP_ATR_VAL:

The OP_ATR_SIZE in ada_evaluate_subexp in current head looks like this:

    case OP_ATR_SIZE:
      arg1 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
      if (noside == EVAL_SKIP)
        goto nosideret;
      else if (noside == EVAL_AVOID_SIDE_EFFECTS)
        return value_zero (builtin_type_int, not_lval);
      else
        return value_from_longest (builtin_type_int,
                                   TARGET_CHAR_BIT
                                   * TYPE_LENGTH (value_type (arg1)));

and after my patch set we have:

    case OP_ATR_SIZE:
      arg1 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
      if (noside == EVAL_SKIP)
        goto nosideret;
      type = builtin_type (exp->gdbarch)->builtin_int;
      if (noside == EVAL_AVOID_SIDE_EFFECTS)
        return value_zero (type, not_lval);
      else
        return value_from_longest (type,
                                   TARGET_CHAR_BIT
                                   * TYPE_LENGTH (value_type (arg1)));

Do you have some other patches applied?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Joel Brobecker]

> >   1. Pointer arithmetics, in particular "PTR + PTR" or "PTR - PTR"
> >      expressions. For instance:
> > 
> >        (gdb) print b'address - a'address
> >        Argument to arithmetic operation not a number or boolean.
[...]
> I see.  In that case, your patch would be a bugfix completely
> independently of my patch set.  Do you want to commit it right away?

Sure! I don't know what this didn't cross my mind earlier, I guess
I was in a bit of a crunch... Will do that tomorrow (I'm also planning
on submitting a bunch of patches tomorrow as well, if things go as
planned - so that'll be a good time for that).


> >   2. The second problem is just an oversight. You needed a variable
> >      to store the int builtin type, and unfortunately you reused
> >      a variable that was still in use.
> >      See ada-lang.c (evaluate_subexp) [OP_ATR_SIZE].
> > 
> >      For now, I just used builtin_type_int32. Not ideal, but should
> >      be large enough for the vast majority of objects we actually
> >      have to deal with in real life.
> 
> Huh?  I'm not sure what base this patch is against:

It was on top of your patch - If I am not mistaken, it should apply
cleanly after you apply yours. I did, however, generate the patch
against AdaCore's gdb-head (which is a merge between our changes
and nearly-top-of-fsf-tree) to which I applied your patch.  So there
might indeed be some differences that would cause a conflict; I just
don't see any, right now.

Perhaps there are some withspace differences? I hate those tabs with a
passion, and always having them right is really difficult and fustrating
for me.

> > --- a/ada-lang.c	Tue Sep 09 12:31:38 2008 -0700
> > +++ b/ada-lang.c	Tue Sep 09 12:57:26 2008 -0700
> > @@ -10541,11 +10541,10 @@ ada_evaluate_subexp (struct type *expect
> >  
> >        if (noside == EVAL_SKIP)
> >          goto nosideret;
> > -      type = builtin_type (exp->gdbarch)->builtin_int;
> >        if (noside == EVAL_AVOID_SIDE_EFFECTS)
> > -        return value_zero (type, not_lval);
> > -      else
> > -        return value_from_longest (type,
> > +        return value_zero (builtin_type_int32, not_lval);
> > +      else
> > +        return value_from_longest (builtin_type_int32,
> >                                     TARGET_CHAR_BIT * TYPE_LENGTH (type));
> >  
> >      case OP_ATR_VAL:
> 
> The OP_ATR_SIZE in ada_evaluate_subexp in current head looks like this:
> 
>     case OP_ATR_SIZE:
>       arg1 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
>       if (noside == EVAL_SKIP)
>         goto nosideret;
>       else if (noside == EVAL_AVOID_SIDE_EFFECTS)
>         return value_zero (builtin_type_int, not_lval);
>       else
>         return value_from_longest (builtin_type_int,
>                                    TARGET_CHAR_BIT
>                                    * TYPE_LENGTH (value_type (arg1)));
> 
> and after my patch set we have:
> 
>     case OP_ATR_SIZE:
>       arg1 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
>       if (noside == EVAL_SKIP)
>         goto nosideret;
>       type = builtin_type (exp->gdbarch)->builtin_int;
>       if (noside == EVAL_AVOID_SIDE_EFFECTS)
>         return value_zero (type, not_lval);
>       else
>         return value_from_longest (type,
>                                    TARGET_CHAR_BIT
>                                    * TYPE_LENGTH (value_type (arg1)));
> 
-- 
Joel

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Ulrich Weigand]

Joel Brobecker wrote:

> Sure! I don't know what this didn't cross my mind earlier, I guess
> I was in a bit of a crunch... Will do that tomorrow (I'm also planning
> on submitting a bunch of patches tomorrow as well, if things go as
> planned - so that'll be a good time for that).

OK, thanks!

> It was on top of your patch - If I am not mistaken, it should apply
> cleanly after you apply yours. I did, however, generate the patch
> against AdaCore's gdb-head (which is a merge between our changes
> and nearly-top-of-fsf-tree) to which I applied your patch.  So there
> might indeed be some differences that would cause a conflict; I just
> don't see any, right now.

Your version has:

> > > +        return value_from_longest (builtin_type_int32,
> > >                                     TARGET_CHAR_BIT * TYPE_LENGTH (type));

which causes the problem with the re-used "type" variable.

The current gdb-head version has:

> >         return value_from_longest (builtin_type_int,
> >                                    TARGET_CHAR_BIT
> >                                    * TYPE_LENGTH (value_type (arg1)));

instead, where this problem does not occur.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc][00/37] Eliminate builtin_type_ macros

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]

> > Sure! I don't know what this didn't cross my mind earlier, I guess
> > I was in a bit of a crunch... Will do that tomorrow (I'm also planning
> > on submitting a bunch of patches tomorrow as well, if things go as
> > planned - so that'll be a good time for that).
> 
> OK, thanks!

OK, I ended up checking in both patches (there was simply too much
confusion on the second one, so it was simpler if I just change
the use of builtin_type_int into builtin_type_int32).  Hopefully,
this should clear the way for you to commit your series of patches!

2008-09-10  Joel Brobecker  <brobecker@adacore.com>

        * ada-lang.c (ada_evaluate_subexp) [BINOP_ADD, BINOP_SUB]:
        Add special handling for pointer types.

2008-09-10  Joel Brobecker  <brobecker@adacore.com>

        * ada-lang.c (ada_evaluate_subexp) [OP_ATR_SIZE]: Use
        archecture-neutral builtin_type_int32 instead of builtin_type_int.

As I said, using int32 is not completely ideal, but should be good
enough in practice. If someone ever hits that limitation, I'll think
about writing a function that returns a type that's big enough to hold
a given LONGEST/ULONGEST...

Actual patches attached. Tested on x86-linux by running the gdb.ada
testcases.

Cheers,
-- 
Joel

[-- Attachment #2: 01-ptrdiff.diff --]
[-- Type: text/plain, Size: 1409 bytes --]

Index: ada-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/ada-lang.c,v
retrieving revision 1.156
retrieving revision 1.157
diff -u -p -r1.156 -r1.157
--- ada-lang.c	10 Sep 2008 09:47:39 -0000	1.156
+++ ada-lang.c	10 Sep 2008 16:12:35 -0000	1.157
@@ -8497,6 +8497,10 @@ ada_evaluate_subexp (struct type *expect
       arg2 = evaluate_subexp_with_coercion (exp, pos, noside);
       if (noside == EVAL_SKIP)
         goto nosideret;
+      if (TYPE_CODE (value_type (arg1)) == TYPE_CODE_PTR)
+        return (value_from_longest
+                 (value_type (arg1),
+                  value_as_long (arg1) + value_as_long (arg2)));
       if ((ada_is_fixed_point_type (value_type (arg1))
            || ada_is_fixed_point_type (value_type (arg2)))
           && value_type (arg1) != value_type (arg2))
@@ -8514,6 +8518,10 @@ ada_evaluate_subexp (struct type *expect
       arg2 = evaluate_subexp_with_coercion (exp, pos, noside);
       if (noside == EVAL_SKIP)
         goto nosideret;
+      if (TYPE_CODE (value_type (arg1)) == TYPE_CODE_PTR)
+        return (value_from_longest
+                 (value_type (arg1),
+                  value_as_long (arg1) - value_as_long (arg2)));
       if ((ada_is_fixed_point_type (value_type (arg1))
            || ada_is_fixed_point_type (value_type (arg2)))
           && value_type (arg1) != value_type (arg2))

[-- Attachment #3: 02-size_atr.diff --]
[-- Type: text/plain, Size: 807 bytes --]

Index: ada-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/ada-lang.c,v
retrieving revision 1.157
diff -u -p -r1.157 ada-lang.c
--- ada-lang.c	10 Sep 2008 16:12:35 -0000	1.157
+++ ada-lang.c	10 Sep 2008 16:19:19 -0000
@@ -9074,9 +9074,9 @@ ada_evaluate_subexp (struct type *expect
       if (noside == EVAL_SKIP)
         goto nosideret;
       else if (noside == EVAL_AVOID_SIDE_EFFECTS)
-        return value_zero (builtin_type_int, not_lval);
+        return value_zero (builtin_type_int32, not_lval);
       else
-        return value_from_longest (builtin_type_int,
+        return value_from_longest (builtin_type_int32,
                                    TARGET_CHAR_BIT
                                    * TYPE_LENGTH (value_type (arg1)));