| Message ID | 005bad3cb21745d591cea63567d08d08@kth.se |
|---|---|
| State | New |
| Headers | show |
| Series | jit : Generate debug info for variables | expand |
Well I seemed to have attached the wrong testcase. Here is the proper one attached. Regards, -----Ursprungligt meddelande----- Från: Petter Tomner Skickat: den 31 augusti 2021 02:14 Till: gcc-patches@gcc.gnu.org; jit@gcc.gnu.org Ämne: [PATCH] jit : Generate debug info for variables Hi, This is a patch to generate debug info for local variables as well as globals. With this, "ptype foo", "info variables", "info locals" etc works when debugging in GDB. Finalizing of global variable declares are moved to after locations are handled and done as Fortran, C, Go etc do it. Also, primitive types have their TYPE_NAME set for debug info on types to work. Below are the patch, and I attached a testcase. Since it requires GDB to run it might not be suitable? Make check-jit runs fine on Debian x64. Regards,
On Tue, 2021-08-31 at 00:13 +0000, Petter Tomner via Gcc-patches wrote: > Hi, > > This is a patch to generate debug info for local variables as well as > globals. > With this, "ptype foo", "info variables", "info locals" etc works > when debugging in GDB. > > Finalizing of global variable declares are moved to after locations > are handled and done > as Fortran, C, Go etc do it. Also, primitive types have their > TYPE_NAME set for debug info > on types to work. > > Below are the patch, and I attached a testcase. Since it requires GDB > to run it might > not be suitable? Make check-jit runs fine on Debian x64. Thanks for the patches. Overall, looks good, but I have some review nits... Reviewing patch 1 in this email... > From d77e77104024c7ae9ce31b419dad1f0a5801fda7 Mon Sep 17 00:00:00 > 2001 > From: Petter Tomner <tomner@kth.se> > Date: Mon, 30 Aug 2021 01:44:07 +0200 > Subject: [PATCH 1/2] libgccjit: Generate debug info for variables > > Finalize declares via available helpers after location is set. Set > TYPE_NAME of primitives and friends to "int" etc. Debug info is now > set properly for variables. > > 2021-08-31 Petter Tomner <tomner@kth.se> > > gcc/jit > jit-playback.c > jit-playback.h > gcc/testsuite/jit.dg/ > test-error-array-bounds.c: Array is not unsigned Can you write non-empty ChangeLog entries please. [...snip...] > --- a/gcc/jit/jit-playback.c > +++ b/gcc/jit/jit-playback.c [...snip...] > @@ -2984,15 +2975,22 @@ replay () > { > int i; > function *func; > - > + tree global; > /* No GC can happen yet; process the cached source locations. > */ > handle_locations (); > > + /* Finalize globals. See how FORTRAN 95 does it in > gfc_be_parse_file() > + for a simple reference. */ > + FOR_EACH_VEC_ELT (m_globals, i, global) > + rest_of_decl_compilation (global, true, true); > + > + wrapup_global_declarations (m_globals.address(), > m_globals.length()); > + > /* We've now created tree nodes for the stmts in the various > blocks > - in each function, but we haven't built each function's > single stmt > - list yet. Do so now. */ > + in each function, but we haven't built each function's > single stmt > + list yet. Do so now. */ > FOR_EACH_VEC_ELT (m_functions, i, func) > - func->build_stmt_list (); > + func->build_stmt_list (); Looks like some whitespace churn above; did your text editor accidentally convert tabs to spaces? I prefer to avoid changes that touch lines without changing things, as it messes up e.g. "git blame". In case you haven't discovered it yet, "git add -p" is very helpful for just staging individual hunks within a changed file. [...snip...] ...plus some comments about the testcase which I'll post in reply to the other patch. Dave
On Tue, 2021-08-31 at 00:23 +0000, Petter Tomner via Gcc-patches wrote: > Well I seemed to have attached the wrong testcase. Here is the proper > one attached. > > Regards, > > -----Ursprungligt meddelande----- > Från: Petter Tomner > Skickat: den 31 augusti 2021 02:14 > Till: gcc-patches@gcc.gnu.org; jit@gcc.gnu.org > Ämne: [PATCH] jit : Generate debug info for variables > > Hi, > > This is a patch to generate debug info for local variables as well as > globals. > With this, "ptype foo", "info variables", "info locals" etc works when > debugging in GDB. > > Finalizing of global variable declares are moved to after locations are > handled and done > as Fortran, C, Go etc do it. Also, primitive types have their TYPE_NAME > set for debug info > on types to work. > > Below are the patch, and I attached a testcase. Since it requires GDB > to run it might > not be suitable? Make check-jit runs fine on Debian x64. > > Regards, > From 6a5d24cbe80429d19042e643bd4c23940cd185fa Mon Sep 17 00:00:00 2001 > From: Petter Tomner <tomner@kth.se> > Date: Mon, 30 Aug 2021 01:45:11 +0200 > Subject: [PATCH 2/2] libgccjit: Test cases for debug info > > Assure that debug info is available for variables. > > gcc/testsuite/jit.dg/ > jit.exp: Helper function > test-debuginfo.c Again, please provided non-empty ChangeLog entries. You can use contrib/gcc-changelog/git_check_commit.py to validate them. I don't see "Signed-off-by" tags in the patches. Have you either filed a copyright assignment with the FSF, or can you please add the tags to certify that you wrote the patches and can contribute them, see: https://gcc.gnu.org/contribute.html#legal https://gcc.gnu.org/dco.html [...snip...] > +proc jit-check-debug-info { obj_file cmds match } { > + verbose "Checking debug info for $obj_file with match: $match" > + > + if { [catch {exec gdb -v} fid] } { > + verbose "No gdb seems to be in path. Can't check debug info. Reporting expected fail." > + xfail "No gdb seems to be in path. Can't check debug info" I think this should be "unsupported" rather than "xfail". [...snip...] > diff --git a/gcc/testsuite/jit.dg/test-debuginfo.c b/gcc/testsuite/jit.dg/test-debuginfo.c > new file mode 100644 > index 00000000000..0af5779fdd1 > --- /dev/null > +++ b/gcc/testsuite/jit.dg/test-debuginfo.c > @@ -0,0 +1,72 @@ > +/* Essentially this test checks that debug info are generated for globals > + locals and functions, including type info. The comment bellow is used > + as fake code (does not affect the test, use for manual debugging). */ > +/* > +int a_global_for_test_debuginfo; > +int main (int argc, char **argv) > +{ > + int a_local_for_test_debuginfo = 2; > + return a_global_for_test_debuginfo + a_local_for_test_debuginfo; > +} > +*/ This is OK, but maybe using gcc_jit_context_dump_to_file with update_locations == 1 might be more sustainable in the long run? See: https://gcc.gnu.org/onlinedocs/jit/topics/locations.html#faking-it OK otherwise. Thanks Dave
Thanks for the review. I will post a revision to the mailing list. I have not filed a copyright assignment with the FSF, but if DCO is enough I'll add it. I also read the mail about the testcase of this patch. Regards, Petter -----Ursprungligt meddelande----- Från: David Malcolm <dmalcolm@redhat.com> Skickat: den 2 september 2021 17:21 Till: Petter Tomner <tomner@kth.se>; gcc-patches@gcc.gnu.org; jit@gcc.gnu.org Ämne: Re: Sv: [PATCH] jit : Generate debug info for variables On Tue, 2021-08-31 at 00:23 +0000, Petter Tomner via Gcc-patches wrote: > Well I seemed to have attached the wrong testcase. Here is the proper > one attached. > > Regards, > > -----Ursprungligt meddelande----- > Från: Petter Tomner > Skickat: den 31 augusti 2021 02:14 > Till: gcc-patches@gcc.gnu.org; jit@gcc.gnu.org > Ämne: [PATCH] jit : Generate debug info for variables > > Hi, > > This is a patch to generate debug info for local variables as well as > globals. > With this, "ptype foo", "info variables", "info locals" etc works when > debugging in GDB. > > Finalizing of global variable declares are moved to after locations > are handled and done as Fortran, C, Go etc do it. Also, primitive > types have their TYPE_NAME set for debug info on types to work. > > Below are the patch, and I attached a testcase. Since it requires GDB > to run it might not be suitable? Make check-jit runs fine on Debian > x64. > > Regards, > From 6a5d24cbe80429d19042e643bd4c23940cd185fa Mon Sep 17 00:00:00 2001 > From: Petter Tomner <tomner@kth.se> > Date: Mon, 30 Aug 2021 01:45:11 +0200 > Subject: [PATCH 2/2] libgccjit: Test cases for debug info > > Assure that debug info is available for variables. > > gcc/testsuite/jit.dg/ > jit.exp: Helper function > test-debuginfo.c Again, please provided non-empty ChangeLog entries. You can use contrib/gcc-changelog/git_check_commit.py to validate them. I don't see "Signed-off-by" tags in the patches. Have you either filed a copyright assignment with the FSF, or can you please add the tags to certify that you wrote the patches and can contribute them, see: https://gcc.gnu.org/contribute.html#legal https://gcc.gnu.org/dco.html [...snip...] > +proc jit-check-debug-info { obj_file cmds match } { > + verbose "Checking debug info for $obj_file with match: $match" > + > + if { [catch {exec gdb -v} fid] } { > + verbose "No gdb seems to be in path. Can't check debug info. Reporting expected fail." > + xfail "No gdb seems to be in path. Can't check debug info" I think this should be "unsupported" rather than "xfail". [...snip...] > diff --git a/gcc/testsuite/jit.dg/test-debuginfo.c b/gcc/testsuite/jit.dg/test-debuginfo.c > new file mode 100644 > index 00000000000..0af5779fdd1 > --- /dev/null > +++ b/gcc/testsuite/jit.dg/test-debuginfo.c > @@ -0,0 +1,72 @@ > +/* Essentially this test checks that debug info are generated for globals > + locals and functions, including type info. The comment bellow is used > + as fake code (does not affect the test, use for manual debugging). */ > +/* > +int a_global_for_test_debuginfo; > +int main (int argc, char **argv) > +{ > + int a_local_for_test_debuginfo = 2; > + return a_global_for_test_debuginfo + a_local_for_test_debuginfo; > +} */ This is OK, but maybe using gcc_jit_context_dump_to_file with update_locations == 1 might be more sustainable in the long run? See: https://gcc.gnu.org/onlinedocs/jit/topics/locations.html#faking-it OK otherwise. Thanks Dave
diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c index 79ac525e5df..74321b0946a 100644 --- a/gcc/jit/jit-playback.c +++ b/gcc/jit/jit-playback.c @@ -165,7 +165,8 @@ gt_ggc_mx () /* Given an enum gcc_jit_types value, get a "tree" type. */ -static tree +tree +playback::context:: get_tree_node_for_type (enum gcc_jit_types type_) { switch (type_) @@ -192,11 +193,7 @@ get_tree_node_for_type (enum gcc_jit_types type_) return short_unsigned_type_node; case GCC_JIT_TYPE_CONST_CHAR_PTR: - { - tree const_char = build_qualified_type (char_type_node, - TYPE_QUAL_CONST); - return build_pointer_type (const_char); - } + return m_const_char_ptr; case GCC_JIT_TYPE_INT: return integer_type_node; @@ -579,10 +576,6 @@ playback::lvalue * playback::context:: global_finalize_lvalue (tree inner) { - varpool_node::get_create (inner); - - varpool_node::finalize_decl (inner); - m_globals.safe_push (inner); return new lvalue (this, inner); @@ -2952,9 +2945,7 @@ replay () { JIT_LOG_SCOPE (get_logger ()); - m_const_char_ptr - = build_pointer_type (build_qualified_type (char_type_node, - TYPE_QUAL_CONST)); + init_types (); /* Replay the recorded events: */ timevar_push (TV_JIT_REPLAY); @@ -2984,15 +2975,22 @@ replay () { int i; function *func; - + tree global; /* No GC can happen yet; process the cached source locations. */ handle_locations (); + /* Finalize globals. See how FORTRAN 95 does it in gfc_be_parse_file() + for a simple reference. */ + FOR_EACH_VEC_ELT (m_globals, i, global) + rest_of_decl_compilation (global, true, true); + + wrapup_global_declarations (m_globals.address(), m_globals.length()); + /* We've now created tree nodes for the stmts in the various blocks - in each function, but we haven't built each function's single stmt - list yet. Do so now. */ + in each function, but we haven't built each function's single stmt + list yet. Do so now. */ FOR_EACH_VEC_ELT (m_functions, i, func) - func->build_stmt_list (); + func->build_stmt_list (); /* No GC can have happened yet. */ @@ -3081,6 +3079,50 @@ location_comparator (const void *lhs, const void *rhs) return loc_lhs->get_column_num () - loc_rhs->get_column_num (); } +/* Initialize the NAME_TYPE of the primitive types as well as some + others. */ +void +playback::context:: +init_types () +{ + /* See lto_init() in lto-lang.c or void visit (TypeBasic *t) in D's types.cc + for reference. If TYPE_NAME is not set, debug info will not contain types */ +#define NAME_TYPE(t,n) \ +if (t) \ + TYPE_NAME (t) = build_decl (UNKNOWN_LOCATION, TYPE_DECL, \ + get_identifier (n), t) + + NAME_TYPE (integer_type_node, "int"); + NAME_TYPE (char_type_node, "char"); + NAME_TYPE (long_integer_type_node, "long int"); + NAME_TYPE (unsigned_type_node, "unsigned int"); + NAME_TYPE (long_unsigned_type_node, "long unsigned int"); + NAME_TYPE (long_long_integer_type_node, "long long int"); + NAME_TYPE (long_long_unsigned_type_node, "long long unsigned int"); + NAME_TYPE (short_integer_type_node, "short int"); + NAME_TYPE (short_unsigned_type_node, "short unsigned int"); + if (signed_char_type_node != char_type_node) + NAME_TYPE (signed_char_type_node, "signed char"); + if (unsigned_char_type_node != char_type_node) + NAME_TYPE (unsigned_char_type_node, "unsigned char"); + NAME_TYPE (float_type_node, "float"); + NAME_TYPE (double_type_node, "double"); + NAME_TYPE (long_double_type_node, "long double"); + NAME_TYPE (void_type_node, "void"); + NAME_TYPE (boolean_type_node, "bool"); + NAME_TYPE (complex_float_type_node, "complex float"); + NAME_TYPE (complex_double_type_node, "complex double"); + NAME_TYPE (complex_long_double_type_node, "complex long double"); + + m_const_char_ptr = build_pointer_type( + build_qualified_type (char_type_node, TYPE_QUAL_CONST)); + + NAME_TYPE (m_const_char_ptr, "char"); + NAME_TYPE (size_type_node, "size_t"); + NAME_TYPE (fileptr_type_node, "FILE"); +#undef NAME_TYPE +} + /* Our API allows locations to be created in arbitrary orders, but the linemap API requires locations to be created in ascending order as if we were tokenizing files. diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h index 825a3e172e9..f670c9e81df 100644 --- a/gcc/jit/jit-playback.h +++ b/gcc/jit/jit-playback.h @@ -271,8 +271,13 @@ private: source_file * get_source_file (const char *filename); + tree + get_tree_node_for_type (enum gcc_jit_types type_); + void handle_locations (); + void init_types (); + const char * get_path_c_file () const; const char * get_path_s_file () const; const char * get_path_so_file () const; diff --git a/gcc/testsuite/jit.dg/test-error-array-bounds.c b/gcc/testsuite/jit.dg/test-error-array-bounds.c index cd9361fba1d..b6c0ee526d4 100644 --- a/gcc/testsuite/jit.dg/test-error-array-bounds.c +++ b/gcc/testsuite/jit.dg/test-error-array-bounds.c @@ -70,5 +70,5 @@ verify_code (gcc_jit_context *ctxt, gcc_jit_result *result) /* ...and that the message was captured by the API. */ CHECK_STRING_VALUE (gcc_jit_context_get_first_error (ctxt), "array subscript 10 is above array bounds of" - " 'unsigned char[10]' [-Warray-bounds]"); + " 'char[10]' [-Warray-bounds]"); }
Hi, This is a patch to generate debug info for local variables as well as globals. With this, "ptype foo", "info variables", "info locals" etc works when debugging in GDB. Finalizing of global variable declares are moved to after locations are handled and done as Fortran, C, Go etc do it. Also, primitive types have their TYPE_NAME set for debug info on types to work. Below are the patch, and I attached a testcase. Since it requires GDB to run it might not be suitable? Make check-jit runs fine on Debian x64. Regards, From d77e77104024c7ae9ce31b419dad1f0a5801fda7 Mon Sep 17 00:00:00 2001 From: Petter Tomner <tomner@kth.se> Date: Mon, 30 Aug 2021 01:44:07 +0200 Subject: [PATCH 1/2] libgccjit: Generate debug info for variables Finalize declares via available helpers after location is set. Set TYPE_NAME of primitives and friends to "int" etc. Debug info is now set properly for variables. 2021-08-31 Petter Tomner <tomner@kth.se> gcc/jit jit-playback.c jit-playback.h gcc/testsuite/jit.dg/ test-error-array-bounds.c: Array is not unsigned --- gcc/jit/jit-playback.c | 76 ++++++++++++++----- gcc/jit/jit-playback.h | 5 ++ .../jit.dg/test-error-array-bounds.c | 2 +- 3 files changed, 65 insertions(+), 18 deletions(-)