| Message ID | 20150327163646.GA16457@host1.jankratochvil.net |
|---|---|
| State | New |
| Headers | show |
Hi, ping: [gcc patch] libcc1: '@' GDB array operator https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01451.html Message-ID: <20150327163646.GA16457@host1.jankratochvil.net> Jan
On 04/17/2015 09:17 AM, Jan Kratochvil wrote: > Hi, > > ping: > [gcc patch] libcc1: '@' GDB array operator > https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01451.html > Message-ID: <20150327163646.GA16457@host1.jankratochvil.net> > > > Jan > Sorry this has taken so long to get back to... We're still digging out a bit from the patches queued while preparing for the gcc5 release. So I guess at some level it's not clear to me why we need to support the @ operator in libcc1. So perhaps starting with a justification for wanting/needed that capability would be helpful. And just to be clear, I'm an avid fan of '@' in gdb, so you can assume I already know what it does and accept that it's a good thing in gdb. As for the patch itself, you noted you weren't sure if copy_node was right, it would help if you'd describe what problem you were having that's solved by copying the node. I wonder if you should be building up a node from scratch here. In general, please use C style comments rather than C++. It's a nit and may change one day, but the general consensus has been to stick with old-style comments for now. Jeff
On Sat, 30 May 2015 07:42:35 +0200, Jeff Law wrote: > We're still digging out a > bit from the patches queued while preparing for the gcc5 release. gcc6 > So I guess at some level it's not clear to me why we need to support the @ > operator in libcc1. So perhaps starting with a justification for > wanting/needed that capability would be helpful. It is not a simple /@[0-9]+$/ regex, the expression can be for example (*vararray@(3+1)) Parentheses still could be parsed by GDB, though. But a statement expression could not be parsed by GDB: compile print ({ __auto_type ptr=vararray+1; *ptr@3; }) But I have found now it does not work - it prints just a pointer, not an array - due to: 1 int main (void) { typeof (({ int a[]={1,2,3,4,5}; a; })) *ptr_type; return 0; } (gdb) ptype ptr_type type = int ** It is in DWARF really just: DW_TAG_pointer_type -> DW_TAG_pointer_type -> DW_TAG_base_type With future C++ support it also would not work: 1 int array[]={1,2,3,4,5}; 2 auto copy(array); (gdb) ptype copy type = int * I have found now GDB can do also *vararray@somevar while this GCC patch cannot: gdb command line:1:39: error: second parameter of operator '@' requires constant integer I did not realize that myself before. I do not think there is an easy fix for the GCC patch, is it? But I do not think it matters too much, IMO GDB users usually put there just constant numbers, at least I do. So all the currently working cases can be implemented also just in GDB. I still find more correct to do it in GCC than to implement new kind of expression parsing in GDB - which the 'compile' project tries to avoid. But sure up to you whether it fits in GCC or not. > As for the patch itself, you noted you weren't sure if copy_node was right, > it would help if you'd describe what problem you were having that's solved > by copying the node. I wonder if you should be building up a node from > scratch here. I have removed it and it works. But there are many statements I do not understand and I only guess they should be copying similar code around. The block of code can be replaced just by: case ATSIGN_EXPR: op0 = TREE_OPERAND (expr, 0); op1 = TREE_OPERAND (expr, 1); ret = op0; TREE_TYPE (ret) = build_array_type_nelts (TREE_TYPE (op0), tree_to_uhwi (op1)); TREE_READONLY (ret) = TREE_READONLY (expr); TREE_SIDE_EFFECTS (ret) = TREE_SIDE_EFFECTS (expr); TREE_THIS_VOLATILE (ret) = TREE_THIS_VOLATILE (expr); goto out; which also works for me. But I guess one could find some countercases for this simplified block. > In general, please use C style comments rather than C++. Done. Jan
On 05/30/2015 03:47 AM, Jan Kratochvil wrote: >> So I guess at some level it's not clear to me why we need to support the @ >> operator in libcc1. So perhaps starting with a justification for >> wanting/needed that capability would be helpful. > > It is not a simple /@[0-9]+$/ regex, the expression can be for example > (*vararray@(3+1)) > Parentheses still could be parsed by GDB, though. Is your assertion here that you want to be able to handle more complex operands on the LHS/RHS of the @ and that adding a general expression parser to GDB for that would be painful/wasteful? > > But a statement expression could not be parsed by GDB: > compile print ({ __auto_type ptr=vararray+1; *ptr@3; }) But how important is this kind of usage? > I have found now GDB can do also > *vararray@somevar Yea. I've used this occasionally, but... > while this GCC patch cannot: > gdb command line:1:39: error: second parameter of operator '@' requires constant integer > I did not realize that myself before. I do not think there is an easy fix for > the GCC patch, is it? But I do not think it matters too much, IMO GDB users > usually put there just constant numbers, at least I do. 99% of the time I've used a constant with the @ syntax in gdb. Doesn't this conflict with the goal of supporting an arbitrary C expression on the LHS/RHS of the @? If most uses for the RHS are just constants, then why do we need the enhancement? Accepting the syntax where the RHS doesn't fold down to a constant is easy. And as long as there's some kind of reasonable capture mechanism (and there must be since you want to access variables from in the inferior fragment), then it ought to "just work" if you relax the test in c-typeck.c part of your patch and use build_array_type rather than build_array_type_nelts in that case. >> As for the patch itself, you noted you weren't sure if copy_node was right, >> it would help if you'd describe what problem you were having that's solved >> by copying the node. I wonder if you should be building up a node from >> scratch here. > > I have removed it and it works. But there are many statements I do not > understand and I only guess they should be copying similar code around. > The block of code can be replaced just by: > case ATSIGN_EXPR: > op0 = TREE_OPERAND (expr, 0); > op1 = TREE_OPERAND (expr, 1); > ret = op0; > TREE_TYPE (ret) = build_array_type_nelts (TREE_TYPE (op0), > tree_to_uhwi (op1)); > TREE_READONLY (ret) = TREE_READONLY (expr); > TREE_SIDE_EFFECTS (ret) = TREE_SIDE_EFFECTS (expr); > TREE_THIS_VOLATILE (ret) = TREE_THIS_VOLATILE (expr); > goto out; > which also works for me. But I guess one could find some countercases for this > simplified block. My worry is that without the copy_node we're changing things inside op0. ISTM that we should be generating a new node (ie, a new VAR_DECL) using the type returned by build_array_type_nelts as its type. Jeff
>> I did not realize that myself before. I do not think there is an >> easy fix for the GCC patch, is it? It seems like a VLA would work. Jeff> 99% of the time I've used a constant with the @ syntax in gdb. I use a non-constant argument to @ quite a bit. It's common to have something like the struct hack where the length of the array is stored in the struct. Then when scripting gdb one can write: print s.array[0] @ s.length Tom
On 06/03/2015 09:29 AM, Tom Tromey wrote: >>> I did not realize that myself before. I do not think there is an >>> easy fix for the GCC patch, is it? > > It seems like a VLA would work. Right. It doesn't seem like a big stretch to me either. > > Jeff> 99% of the time I've used a constant with the @ syntax in gdb. > > I use a non-constant argument to @ quite a bit. > > It's common to have something like the struct hack where the length of > the array is stored in the struct. Then when scripting gdb one can > write: > > print s.array[0] @ s.length Which is case that I've had use for a non-constant RHS as well. I just haven't had to use it all that much. Jeff
On 03/06/15 18:30, Jeff Law wrote: >> It's common to have something like the struct hack where the length of >> the array is stored in the struct. Then when scripting gdb one can >> write: >> >> print s.array[0] @ s.length > Which is case that I've had use for a non-constant RHS as well. I just > haven't had to use it all that much. To implement that case, you still do not need the compiler to parse @, you just need the compiler to parse s.array[0] and s.length independently. It should be possible to arrange the inferior code in such a way that GCC parses each side of @ independently and gives the info necessary to GDB such that it can interpret what @ means or give a reasonable error. I discussed this at length here: https://gcc.gnu.org/ml/gcc/2015-03/msg00187.html At https://gcc.gnu.org/ml/gcc/2015-03/msg00183.html I propose other alternatives, which in principle are more work to implement, but move GCC in the direction of being able to parse snippets of code. In my opinion this is moving backwards: making GCC less flexible and adding very special cases that are likely to conflict with other things. Parsing correctly arbitrary programs that may contain @ at arbitrary places seems a can full of gigantic were-worms. Cheers, Manuel.
Manuel> It should be possible to arrange the inferior code in Manuel> such a way that GCC parses each side of @ independently and gives the Manuel> info necessary to GDB such that it can interpret what @ means or give Manuel> a reasonable error. Only if this can be done without requiring gdb to lex the expression. Manuel> Parsing correctly arbitrary programs that may contain @ at arbitrary Manuel> places seems a can full of gigantic were-worms. I think you are overstating the case. This would be an extension only visible via the special gdb plugin, not a general use extension. And, if at some future date C and/or C++ add a use of "@", gdb is going to have to change anyway. So I don't think it is so bad. The real problems are likely the other gdb extensions that are syntactically even more questionable; plus the protection-bypassing hack for C++. That's right -- look over there! Tom
On 3 June 2015 at 22:58, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: > In general parsing LHS vs. RHS is not so trivial: > *array1@10 > expression wrapped into -> > (*array2+"a@c"[1]+'@'+'\''@(*array1@10)[5])[2] > Is this a real case? I cannot understand what this means, but it could simply be that I'm only familiar with the most basic uses of @ together with 'print'. In any case, any option is better than no implementation, the decision is not up to me, and I don't have the time to implement the alternative I propose myself. Cheers, Manuel.
On Thu, 04 Jun 2015 09:24:36 +0200, Manuel López-Ibáñez wrote: > On 3 June 2015 at 22:58, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: > > > In general parsing LHS vs. RHS is not so trivial: > > *array1@10 > > expression wrapped into -> > > (*array2+"a@c"[1]+'@'+'\''@(*array1@10)[5])[2] > > > > Is this a real case? I cannot understand what this means, but it could > simply be that I'm only familiar with the most basic uses of @ > together with 'print'. I made a mistake there but unimportant one, let's say the expression is: "a@c"[1]+'@'+'\''+(*array2@(*array1@10)[5])[2] I do not find splitting this expression to LHS@RHS in GDB to be easy. These two expressions are equivalent for all operations except of sizeof(): pointer (*pointer@ANYTHING) So the expression "a@c"[1]+'@'+'\''+(*array2@(*array1@10)[5])[2] is equivalent to: "a@c"[1]+'@'+'\''+(*array2@ array1 [5])[2] is equivalent to: "a@c"[1]+'@'+'\''+ array2 [2] There can be a GDB function which takes X and Y and evaluates: "a@c"[1]+'@'+'\''+(*X @ Y [5])[2] There can be a GDB function which takes X and Y and evaluates: (*X @Y ) With these constructs I want to say that it is not easy to make the LHS@RHS split in GDB without limiting valid expressions it can accept. Jan
On 4 June 2015 at 09:36, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: > On Thu, 04 Jun 2015 09:24:36 +0200, Manuel López-Ibáñez wrote: >> On 3 June 2015 at 22:58, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: > These two expressions are equivalent for all operations except of sizeof(): > pointer > (*pointer@ANYTHING) > Sure, but is this case really that important? After all, the user can type just 'pointer' and in this case @ANYTHING does not matter at all. In the case of sizeof(), the user can also use sizeof(*pointer) * ANYTHING. Moreover, one can always do: (gdb) p a[0]@5 $10 = "somet" (gdb) p $10 + 5 $11 = 0x80484c5 <a+5> "ignoeisopjffaldjkñfqñlkjfel" My understanding is that there is nothing one can get by means of @ that one cannot get with standard C, except for printing a memory region, and for that purpose one only needs to parse LHS@RHS and only one @ makes sense within the same print command. Again, I may be unaware of other uses of @, but no case shown so far has convinced me otherwise. > With these constructs I want to say that it is not easy to make the LHS@RHS > split in GDB without limiting valid expressions it can accept. Let's leave it then at my limited experience with GDB then, since I cannot imagine why anyone who is not a GDB hacker who understands the fine details of @ would think about writing such expression rather than plain C/C++. :) Thanks for your patience, Manuel.
On Thu, 04 Jun 2015 10:36:46 +0200, Manuel López-Ibáñez wrote: > except for printing a memory > region, and for that purpose one only needs to parse LHS@RHS and only > one @ makes sense within the same print command. Yes, just LHS or RHS can be pretty complicated containing the '@' character at least in strings / character constants. I agree that GDB-side parser will work in almost all cases, just one can then always find some awkward countercase. Jan
On Thu, Jun 04, 2015 at 10:36:46AM +0200, Manuel López-Ibáñez wrote: > On 4 June 2015 at 09:36, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: > > On Thu, 04 Jun 2015 09:24:36 +0200, Manuel López-Ibáñez wrote: > >> On 3 June 2015 at 22:58, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: > > These two expressions are equivalent for all operations except of sizeof(): > > pointer > > (*pointer@ANYTHING) > > > > Sure, but is this case really that important? After all, the user can > type just 'pointer' and in this case @ANYTHING does not matter at all. > In the case of sizeof(), the user can also use sizeof(*pointer) * > ANYTHING. > > Moreover, one can always do: > > (gdb) p a[0]@5 > $10 = "somet" > (gdb) p $10 + 5 > $11 = 0x80484c5 <a+5> "ignoeisopjffaldjkñfqñlkjfel" > > My understanding is that there is nothing one can get by means of @ > that one cannot get with standard C, except for printing a memory > region, and for that purpose one only needs to parse LHS@RHS and only > one @ makes sense within the same print command. Again, I may be > unaware of other uses of @, but no case shown so far has convinced me > otherwise. Even for printing, can't you just use a standard C/C++ construct instead (perhaps violating aliasing rule, but that shouldn't be an issue in gdb)? int main () { int a[] = { 1, 2, 3, 4, 5 }; char b[] = "abcdefg"; return 0; } (gdb) p *(int (*)[4])&a[0] $1 = {1, 2, 3, 4} (gdb) p *(char (*)[4])&b[1] $2 = "bcde" Though, admittedly that is more typing than a[0]@4 or b[1]@4 . If you want to parse @, supposedly you want to transform it during parsing to the taking address, cast to pointer to an array and finally dereferencing, perhaps with ref_all pointer. Jakub
On Thu, 04 Jun 2015 10:55:59 +0200, Jakub Jelinek wrote: > (gdb) p *(int (*)[4])&a[0] > $1 = {1, 2, 3, 4} > (gdb) p *(char (*)[4])&b[1] > $2 = "bcde" > > Though, admittedly that is more typing than a[0]@4 or b[1]@4 . I forgot during this discussion about the C style cast, you are right. For some GDB functions needing to print arrays, one can use: *POINTER@LENGTH -> *(typeof(*(POINTER))(*)[LENGTH])POINTER So that one can even extract type of the expression POINTER. So that @ operator is only for user convenience and I agree it can be implemented by LHS@RHS splitting on GDB side and some unsupported corner cases do not matter much. Jan
On 06/04/2015 01:36 AM, Jan Kratochvil wrote: > On Thu, 04 Jun 2015 09:24:36 +0200, Manuel López-Ibáñez wrote: >> On 3 June 2015 at 22:58, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: >> >>> In general parsing LHS vs. RHS is not so trivial: >>> *array1@10 >>> expression wrapped into -> >>> (*array2+"a@c"[1]+'@'+'\''@(*array1@10)[5])[2] >>> >> >> Is this a real case? I cannot understand what this means, but it could >> simply be that I'm only familiar with the most basic uses of @ >> together with 'print'. > > I made a mistake there but unimportant one, let's say the expression is: > "a@c"[1]+'@'+'\''+(*array2@(*array1@10)[5])[2] > I do not find splitting this expression to LHS@RHS in GDB to be easy. > > These two expressions are equivalent for all operations except of sizeof(): > pointer > (*pointer@ANYTHING) > > So the expression > "a@c"[1]+'@'+'\''+(*array2@(*array1@10)[5])[2] > is equivalent to: > "a@c"[1]+'@'+'\''+(*array2@ array1 [5])[2] > is equivalent to: > "a@c"[1]+'@'+'\''+ array2 [2] > There can be a GDB function which takes X and Y and evaluates: > "a@c"[1]+'@'+'\''+(*X @ Y [5])[2] > There can be a GDB function which takes X and Y and evaluates: > (*X @Y ) > > > With these constructs I want to say that it is not easy to make the LHS@RHS > split in GDB without limiting valid expressions it can accept. But my assertion is that stuff like what you've shown above simply isn't important to handle. What we need to look at are the common cases and I haven't seen a strong argument that the common cases can't be handled by gdb. jeff
On Thu, 04 Jun 2015 16:00:18 +0200, Jeff Law wrote: > But my assertion is that stuff like what you've shown above simply isn't > important to handle. What we need to look at are the common cases and I > haven't seen a strong argument that the common cases can't be handled by > gdb. If we target only 99% of cases then sure GDB-side is enough. Still the GDB side will be more code and IMHO at an inappropriate place. I was tought + expect that in GNU world it does not matter where a feature is implemented, it is more important to be implemented at the right place. All the reasons have been already exchanged and I read between lines GCC still does not want to accept this feature so I will reimplement it in GDB only. Jan
On 06/04/2015 08:27 AM, Jan Kratochvil wrote: > On Thu, 04 Jun 2015 16:00:18 +0200, Jeff Law wrote: >> But my assertion is that stuff like what you've shown above simply isn't >> important to handle. What we need to look at are the common cases and I >> haven't seen a strong argument that the common cases can't be handled by >> gdb. > > If we target only 99% of cases then sure GDB-side is enough. > > Still the GDB side will be more code and IMHO at an inappropriate place. > I was tought + expect that in GNU world it does not matter where a feature is > implemented, it is more important to be implemented at the right place. You're looking to add a lot of generality to this GDB feature, but I'm not convinced that generality is ultimately going to be useful. Thus, I'm not sure that a right place exists at all. jeff
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 456c619..37cb104c 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -1289,6 +1289,28 @@ c_fully_fold_internal (tree expr, bool in_init, bool *maybe_const_operands, ret = fold (ret); goto out; + case ATSIGN_EXPR: + orig_op0 = op0 = TREE_OPERAND (expr, 0); + orig_op1 = op1 = TREE_OPERAND (expr, 1); + op0 = c_fully_fold_internal (op0, in_init, maybe_const_operands, + maybe_const_itself); + STRIP_TYPE_NOPS (op0); + op1 = c_fully_fold_internal (op1, in_init, maybe_const_operands, + maybe_const_itself); + STRIP_TYPE_NOPS (op1); + op1 = decl_constant_value_for_optimization (op1); + ret = copy_node (op0); + TREE_TYPE (ret) = build_array_type_nelts (TREE_TYPE (op0), + tree_to_uhwi (op1)); + if (ret != expr) + { + TREE_READONLY (ret) = TREE_READONLY (expr); + TREE_SIDE_EFFECTS (ret) = TREE_SIDE_EFFECTS (expr); + TREE_THIS_VOLATILE (ret) = TREE_THIS_VOLATILE (expr); + } + ret = fold (ret); + goto out; + case COMPOUND_EXPR: case MODIFY_EXPR: case PREDECREMENT_EXPR: @@ -4084,6 +4106,8 @@ binary_op_error (location_t location, enum tree_code code, opname = "||"; break; case BIT_XOR_EXPR: opname = "^"; break; + case ATSIGN_EXPR: + opname = "@"; break; default: gcc_unreachable (); } diff --git a/gcc/c-family/c-lex.c b/gcc/c-family/c-lex.c index bb55be8..3a9f968 100644 --- a/gcc/c-family/c-lex.c +++ b/gcc/c-family/c-lex.c @@ -515,7 +515,10 @@ c_lex_with_flags (tree *value, location_t *loc, unsigned char *cpp_flags, break; } - /* FALLTHROUGH */ + // Create ATSIGN_EXPR for GDB. + *value = NULL_TREE; + break; + case CPP_HASH: case CPP_PASTE: { diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c index 5cc3892..2a30d72 100644 --- a/gcc/c/c-parser.c +++ b/gcc/c/c-parser.c @@ -1173,6 +1173,7 @@ enum c_parser_prec { PREC_EQ, PREC_REL, PREC_SHIFT, + PREC_ATSIGN, PREC_ADD, PREC_MULT, NUM_PRECS @@ -6283,6 +6284,16 @@ c_parser_binary_expression (c_parser *parser, struct c_expr *after, oprec = PREC_ADD; ocode = MINUS_EXPR; break; + case CPP_ATSIGN: + if (!c_binding_oracle) + { + error_at (c_parser_peek_token (parser)->location, + "stray %qs in program", "@"); + goto out; + } + oprec = PREC_ATSIGN; + ocode = ATSIGN_EXPR; + break; case CPP_LSHIFT: oprec = PREC_SHIFT; ocode = LSHIFT_EXPR; diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c index ebe4c73..22e7bd8 100644 --- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -10976,6 +10976,19 @@ build_binary_op (location_t location, enum tree_code code, maybe_warn_bool_compare (location, code, orig_op0, orig_op1); break; + case ATSIGN_EXPR: + if (TREE_CODE (orig_op1) == INTEGER_CST) + { + result_type = build_array_type_nelts (type0, tree_to_uhwi (orig_op1)); + converted = 1; + break; + } + // Otherwise it would look unclear: + // error: invalid operands to binary @ (have ‘int’ and ‘int’) + error_at (location, + "second parameter of operator %<@%> requires constant integer"); + return error_mark_node; + default: gcc_unreachable (); } diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c index d7c049f..cf00457 100644 --- a/gcc/tree-pretty-print.c +++ b/gcc/tree-pretty-print.c @@ -2037,6 +2037,7 @@ dump_generic_node (pretty_printer *pp, tree node, int spc, int flags, case LTGT_EXPR: case ORDERED_EXPR: case UNORDERED_EXPR: + case ATSIGN_EXPR: { const char *op = op_symbol (node); op0 = TREE_OPERAND (node, 0); @@ -3432,6 +3433,9 @@ op_symbol_code (enum tree_code code) case MIN_EXPR: return "min"; + case ATSIGN_EXPR: + return "@"; + default: return "<<< ??? >>>"; } diff --git a/gcc/tree.def b/gcc/tree.def index b4b4164..250b8d9 100644 --- a/gcc/tree.def +++ b/gcc/tree.def @@ -669,6 +669,9 @@ DEFTREECODE (PLUS_EXPR, "plus_expr", tcc_binary, 2) DEFTREECODE (MINUS_EXPR, "minus_expr", tcc_binary, 2) DEFTREECODE (MULT_EXPR, "mult_expr", tcc_binary, 2) +/* GDB operator '@' to create array types. */ +DEFTREECODE (ATSIGN_EXPR, "atsign_expr", tcc_binary, 2) + /* Pointer addition. The first operand is always a pointer and the second operand is an integer of type sizetype. */ DEFTREECODE (POINTER_PLUS_EXPR, "pointer_plus_expr", tcc_binary, 2)