On Tue, Mar 9, 2021 at 12:29 PM Jose E. Marchesi <jose.march...@oracle.com> wrote: > > > Hi Richard. > Thanks for looking at this :) > > [Today I sent a V4 of the series containing a couple of fixes to the BTF > code. It doesn't contain big changes so the discussion below still > applies.] > > >> In turn, debug_format_do_cu traverses the tree of DIEs passed to it calling > >> ctf_do_die on them. > >> > >> This conforms the debug format API: > >> > >> FOO_debug_init () > >> Initialize the debug format FOO. > >> > >> FOO_debug_finalize (FILENAME) > >> Possibly write out, cleanup and finalization for debug format FOO. > >> > >> FOO_do_die (DIE) > >> Process the given DIE. > >> > >> Note how the emission of DWARF is interrupted after that point, if no > >> DWARF was requested by the user. > > > > I suppose since we're now supposed to write C++ the above fits > > a RAII style > > > > if (ctf_debug_info_level > ...) > > { > > ctf_debug ctf (filename); > > ctf.do_cu (comp_unit_die ()); > > for (...) > > ctf.do_cu (node->die); > > } > > > > but then I have no strong feelings about this. > > We could turn it into a class, sure. > > >> dwarf2out - dwarf2ctf > >> ===================== > >> > >> The functions ctf_debug_init, ctf_do_die and ctf_debug_finalize, that > >> implement the API described above, are all in gcc/dwarf2ctf.c. > >> > >> Obviously, these routines need access to the dwarf DIE data > >> structures, and several functions which are defined in dwarf2out.[ch], > >> many (most?) of which are private to that file: dw_die_ref, get_AT, > >> etc. > >> > >> Therefore, in this implementation we opted by writing dwarf2ctf.c in a > >> way it can just be #included in dwarf2ctf.c. > >> > >> A question remains: would it be better to abstract these types and > >> functions in an API in dwarf2out.h? > > > > I think that refactoring the big dwarf2out.c would be indeed interesting. > > Not only exposing more details from dwarf2out.h (though we could start > > there). One of the reason it's all private to dwarf2out.c is likely > > optimization (somewhat moot when you LTO GCC nowadays). > > > > So one interesting question is what's the API details CTF/BTF need? > > In dwarf2ctf we use the following stuff defined in dwarf2out.c, which > are functions providing access to the DIE attributes: > > - get_AT > - get_AT_ref > - get_AT_string > - AT_class > - AT_unsigned > > So what if we start by adding non-static version of these functions and > export them in dwarf2out.h: > > - dw_die_get_AT > - dw_die_get_AT_ref > - dw_die_get_AT_string > - dw_die_AT_class > - dw_die_AT_unsigned
The latter two would be dw_AT_class and dw_AT_unsigned (operate on attributes, not on dies). There's the option to move all ^AT_* functions to some dwarf2attrs.h as inline functions (inline in a way they get comdat across modules) and/or make them member functions of dw_attr_node (but that's bigger refactoring unless we keep function-style wrappers in dwarf2out.c). Having two names for the same thing doesn't look too appealing in the long term at least. Just exporting the non-mangled current names from some dwarf2int.h header might also work to avoid the .c #include. > ? > > > We might move data structures and basic accessors to > > dwarf2impl.{c,h} (as opposed to the already existing dwarf2.h). > > Or some better name (or as said, simply keep it in dwarf2out.c for now). > > > > Including dwarf2ctf.c is a bit ugly. > > It is definitely ugly yes, but we didn't want to jump factoring an API > for dwarf2out without discussing it here first :) Yes, much appreciated. I don't think it's a blocker for CTF, but of course. > >> Command line options for debug formats > >> ====================================== > >> > >> This implementation adds the following command-line options to select the > >> emission of CTF and BTF: > >> > >> -gctf[123] > >> -gbtf[123] > >> > >> These options mimic the -g[123...] options for DWARF. > > > > Do -gcft and -gdwarf mix? That is, can I have both in the same ELF > > file? Since that doesn't work for -gdwarf -gstabs for example I wonder > > if we need to make this somehow different? > > The idea is for them to mix, yes. > > >> This involved adding new entries for debug_info_type: > >> > >> CTF_DEBUG - Write CTF debug info. > >> BTF_DEBUG - Write BTF debug info. > >> CTF_AND_DWARF2_DEBUG - Write both CTF and DWARF info. > >> > >> Doing this, we just followed the trend initiated by vmsdbgout.c, which > >> added VMS_DEBUG and VMS_AND_DWARF2_DEBUG. > >> > >> This approach is not very good, because debug_info_type was designed > >> to cover different debug hook implementations; debug formats, in > >> contrast, are a different thing. > >> > >> This translates to problems and fragile behavior: > >> > >> - Everywhere write_symbols is checked we have to expand the logic to > >> take the CTF values into account. You can see that is the case in > >> this patch series. This is very fragile and doesn't scale well: we > >> are most probably missing some checks. > > > > Would be nice to abstract most of the checks. Like if the check > > is for "do I need to generate DWARF DIEs" then use > > need_dwarf_dies (), if it is for "do I want to output DWARF2 debug" then > > check output_dwarf (). I see 64 places that check write_symbols, > > most in vmsdbgout.c ... > > > > Inventing predicates for the existing uses with your new uses in mind > > could make future changes easier at least. > > Makes sense, we will look into abstracting these checks in suitable > predicates. That certainly will improve things :) Thanks. > >> - Backends select what debug hooks to use by defining constants like > >> DWARF2_DEBUGGING_INFO. Since the new debug formats are based on the > >> DWARF debug hooks, that is the constant to define by the backends > >> wanting to support DWARF + debug infos. > >> > >> However, some backends may want to use one of the debug formats by > >> default, i.e. for -g. This is the case of the BPF backend, that > >> needs to generate BTF instead of DWARF. Currently, there is no way > >> to specify this. > > > > It probably should define BTF_DEBUGGING_INFO and that should > > enable parts guarded with DWARF2_DEBUGGING_INFO as well. > > Ok, so from a backend perspective there won't be any difference between > formats based on the DWARF debug hooks and (to deprecate) formats using > their own set of debug hooks... Well, it's a topic that deserves extra thoughts when we actually start deprecating things ... Richard.