On Tue, May 11, 2021 at 4:45 PM Indu Bhagat <indu.bha...@oracle.com> wrote:
>
> On 5/10/21 6:11 AM, Richard Biener wrote:
> > On Thu, May 6, 2021 at 2:31 AM Indu Bhagat via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> To support multiple debug formats, we need to move away from explicit
> >> enumeration of each individual combination of debug formats.
> >
> > debug_set_names with its static buffer seems unused? You wire quite some
> > APIs with gcc_assert on having a single bit set - that doesn't look forward
> > looking.
> >
> > I suppose the BTF followups will "fix" this, but see comments below.
>
> Thanks for your feedback.
>
> Yes, I intended to fix them when I added the CTF/BTF as then I would
> have a way to debug/test it more meaningfully. Because of this, the
> debug_set_names and the associated static buffer were unused in V1 indeed.
>
> For V2, taking your inputs, I have now fixed the uses in c-opts.c and
> c-pch.c at least.
>
> >
> >> gcc/c-family/ChangeLog:
> >>
> >> * c-opts.c (c_common_post_options): Adjust access to
> >> debug_type_names.
> >> * c-pch.c (struct c_pch_validity): Use type uint32_t.
> >> (pch_init): Renamed member.
> >> (c_common_valid_pch): Adjust access to debug_type_names.
> >>
> >> gcc/ChangeLog:
> >>
> >> * common.opt: Change type to support bitmasks.
> >> * flag-types.h (enum debug_info_type): Rename enumerator
> >> constants.
> >> (NO_DEBUG): New bitmask.
> >> (DBX_DEBUG): Likewise.
> >> (DWARF2_DEBUG): Likewise.
> >> (XCOFF_DEBUG): Likewise.
> >> (VMS_DEBUG): Likewise.
> >> (VMS_AND_DWARF2_DEBUG): Likewise.
> >> * flags.h (debug_set_to_format): New function declaration.
> >> (debug_set_count): Likewise.
> >> (debug_set_names): Likewise.
> >> * opts.c (debug_type_masks): Array of bitmasks for debug formats.
> >> (debug_set_to_format): New function definition.
> >> (debug_set_count): Likewise.
> >> (debug_set_names): Likewise.
> >> (set_debug_level): Update access to debug_type_names.
> >> * toplev.c: Likewise.
> >>
> >> gcc/objc/ChangeLog:
> >>
> >> * objc-act.c (synth_module_prologue): Use uint32_t instead of enum
> >> debug_info_type.
> >> ---
> >> gcc/c-family/c-opts.c | 10 +++--
> >> gcc/c-family/c-pch.c | 12 +++---
> >> gcc/common.opt | 2 +-
> >> gcc/flag-types.h | 29 ++++++++++----
> >> gcc/flags.h | 17 +++++++-
> >> gcc/objc/objc-act.c | 2 +-
> >> gcc/opts.c | 109
> >> +++++++++++++++++++++++++++++++++++++++++++++-----
> >> gcc/toplev.c | 9 +++--
> >> 8 files changed, 158 insertions(+), 32 deletions(-)
> >>
> >> diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c
> >> index 89e05a4..e463240 100644
> >> --- a/gcc/c-family/c-opts.c
> >> +++ b/gcc/c-family/c-opts.c
> >> @@ -1112,9 +1112,13 @@ c_common_post_options (const char **pfilename)
> >> /* Only -g0 and -gdwarf* are supported with PCH, for other
> >> debug formats we warn here and refuse to load any PCH files.
> >> */
> >> if (write_symbols != NO_DEBUG && write_symbols != DWARF2_DEBUG)
> >> - warning (OPT_Wdeprecated,
> >> - "the %qs debug format cannot be used with "
> >> - "pre-compiled headers",
> >> debug_type_names[write_symbols]);
> >> + {
> >> + gcc_assert (debug_set_count (write_symbols) <= 1);
> >
> > Why this assert? Iff then simply include the count check in the
> > condition of the warning.
> >
> >> + warning (OPT_Wdeprecated,
> >> + "the %qs debug format cannot be used with "
> >> + "pre-compiled headers",
> >> + debug_type_names[debug_set_to_format
> >> (write_symbols)]);
> >
> > Maybe simply emit another diagnostic if debug_set_count > 1.
> >
>
> OK. I have removed the asserts. The code now uses debug_set_names
> uniformly. I have changed the diagnostic message, as there can be
> multiple debug formats for PCH at some point. So,
>
> from "the 'XXX' debug format cannot be used with pre-compiled headers"
> to "the 'XXX YYY' debug info cannot be used with pre-compiled headers"
> if multiple debug formats were enabled.
>
> >> + }
> >> }
> >> else if (write_symbols != NO_DEBUG && write_symbols !=
> >> DWARF2_DEBUG)
> >> c_common_no_more_pch ();
> >> diff --git a/gcc/c-family/c-pch.c b/gcc/c-family/c-pch.c
> >> index fd94c37..6804388 100644
> >> --- a/gcc/c-family/c-pch.c
> >> +++ b/gcc/c-family/c-pch.c
> >> @@ -52,7 +52,7 @@ enum {
> >>
> >> struct c_pch_validity
> >> {
> >> - unsigned char debug_info_type;
> >> + uint32_t pch_write_symbols;
> >> signed char match[MATCH_SIZE];
> >> void (*pch_init) (void);
> >> size_t target_data_length;
> >> @@ -108,7 +108,7 @@ pch_init (void)
> >> pch_outfile = f;
> >>
> >> memset (&v, '\0', sizeof (v));
> >> - v.debug_info_type = write_symbols;
> >> + v.pch_write_symbols = write_symbols;
> >> {
> >> size_t i;
> >> for (i = 0; i < MATCH_SIZE; i++)
> >> @@ -252,13 +252,15 @@ c_common_valid_pch (cpp_reader *pfile, const char
> >> *name, int fd)
> >> /* The allowable debug info combinations are that either the PCH file
> >> was built with the same as is being used now, or the PCH file was
> >> built for some kind of debug info but now none is in use. */
> >> - if (v.debug_info_type != write_symbols
> >> + if (v.pch_write_symbols != write_symbols
> >> && write_symbols != NO_DEBUG)
> >> {
> >> + gcc_assert (debug_set_count (v.pch_write_symbols) <= 1);
> >> + gcc_assert (debug_set_count (write_symbols) <= 1);
> >
> > So the read-in PCH will have at most one bit set but I don't think
> > you can assert on write_symbols here.
> >
>
> I switched both to use debug_set_names.
>
> > Otherwise looks OK. Did you check for write_symbols uses in FEs and
> > targets?
> >
> > Richard.
>
> Yes, I have. I must admit I have gone back and forth in my mind on this.
> My initial thinking was to adjust only those checks where I expect more
> than 1 write_symbols. Because in most contexts, e.g., (write_symbols ==
> XCOFF_DEBUG) is a stricter check than (write_symbols & XCOFF_DEBUG), in
> some sense.
>
> Anyway, after you raised this point, however, I looked again, and have
> added some dwarf_debuginfo_p usages in the second patch. Basically, I
> replaced some write_symbols == DWARF2_DEBUG with dwarf_debuginfo_p ()
> usage. Other than DWARF2_DEBUG, at this time, no other debug format is
> expected to be written out together with other debug formats.
>
> As for other usages of write_symbols
> - I plan to get rid of the VMS_AND_DWARF2_DEBUG symbol in a subsequent
> patch (this should deal with most usages in vmsdbgout.c).
>
> I will post V2 next.
>
> A question though - I have regression tested the V2 patch set on x86_64.
> Although the sort of changes in the backend files can be OK, I am happy
> to take any further suggestions for testing this patch set.
If you touch backends in non-trivial ways it's at least good to test it
still builds in a simple cross configuration which means
configuring for one of the supported target triplets and doing
make all-gcc (that only builds host objects so you don't need
binutils or target headers for this)
Richard.
> Thanks
> Indu
