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.
Thanks
Indu
