From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 4A5C33858412 for ; Wed, 14 Dec 2022 16:00:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4A5C33858412 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1671033624; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=P9gt4ADuHXcvRanHUk3r7oyJtCQzGnM9mZsUCA/ak14=; b=cfauZYy95/i0RyXpEV+nwU+HegEgEC/y8g2dbFLUnuIe5p4f14Es3cEBelNKjRWlAII2pY z5IUkrKCQem0fK+r+lYaZprFNb6Bg8/4IDfrjo5zaGukjR4r7uZ/mItcWsF8TQGqvDCdkn 50VWBDR1W78Mqo9+34R4qBVkbAejdT8= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-83-_GnT0P4VMZy0Wi-fY3XS2w-1; Wed, 14 Dec 2022 11:00:23 -0500 X-MC-Unique: _GnT0P4VMZy0Wi-fY3XS2w-1 Received: by mail-wm1-f70.google.com with SMTP id bi19-20020a05600c3d9300b003cf9d6c4016so887593wmb.8 for ; Wed, 14 Dec 2022 08:00:23 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=bcFsNQzpEDDxAy/WfiXXV01ANPRKstQJAkiN51YGsj4=; b=oZ8bLLdFWHNbV7R8M4Y7pYruAxhF6g8BngVz2ZM5W8LCKVLQRuKZPpU2hJP7CPHE8/ l24PLoXYE6CdRfT/a0Wdx5bU3t5hiV39Sy8DjVnzuhZrKpYjl68iI/zM8nH7cad8Nk7T qKmPMr+nb58TMjmJxTsALiAaUBDEzvoyPIxtsk9PD5QhGQjkj3qrHuI67pzT3ffCjFws a6he+knlwqKG0gv1DiNwvwsQZz2WUslPGXVsIdArjTKHFNIfurkGJvweAHOaTu6ygwH7 XYaa9vu14+/uSCIZxzvODMj3ANxnb09YmXklogaUhN59qHdWYtUTb8bdBWN2CaLfQGK8 mpbw== X-Gm-Message-State: ANoB5plMobWcSkm40Crwceot1EuLAGvvP/Ft87MdkN0pzFwy0LiOHhYM 6aup6s2G/ya9S5S08uGIyXsQPyKQwBCP4F390Ob6aQuYuC6Np2T+gybzpvSfLa5IBK0IUbuBtUh EWl3itBfYnpE1Z4lDOA== X-Received: by 2002:a5d:4103:0:b0:242:cdf:ab91 with SMTP id l3-20020a5d4103000000b002420cdfab91mr15053111wrp.56.1671033622410; Wed, 14 Dec 2022 08:00:22 -0800 (PST) X-Google-Smtp-Source: AA0mqf4vhf2roHflQjqkUtWJPzezdGtqWL06eRAyKJ8U/dBQmZMdltFsxEv8y0brlx7ZMa+sRkPx3Q== X-Received: by 2002:a5d:4103:0:b0:242:cdf:ab91 with SMTP id l3-20020a5d4103000000b002420cdfab91mr15053083wrp.56.1671033622039; Wed, 14 Dec 2022 08:00:22 -0800 (PST) Received: from localhost ([31.111.84.238]) by smtp.gmail.com with ESMTPSA id b8-20020a05600003c800b0024258722a7fsm3380829wrg.37.2022.12.14.08.00.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 14 Dec 2022 08:00:21 -0800 (PST) From: Andrew Burgess To: Luis Machado , Jan Beulich , Binutils Cc: Nick Clifton , "ramana.radhakrishnan@arm.com" , Richard Earnshaw , Marcus Shawcroft , Alan Modra , Peter Bergner , Geoff Keating , Palmer Dabbelt , Andrew Waterman , Jim Wilson , Nelson Chu , "H.J. Lu" Subject: Re: [PATCH v2 2/2] gas: re-work line number tracking for macros and their expansions In-Reply-To: References: <1d528267-9450-12c7-4c4c-fe4deb3b0617@suse.com> <8eff1de6-871d-24cc-8804-9af7da0a86cf@suse.com> Date: Wed, 14 Dec 2022 16:00:20 +0000 Message-ID: <87edt20ycr.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-1.3 required=5.0 tests=BAYES_00,DKIM_INVALID,DKIM_SIGNED,KAM_DMARC_NONE,KAM_DMARC_STATUS,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Luis Machado via Binutils writes: > Hi, > > I'm not yet sure how, but this patch has broken at least one gdb test (gdb.asm/asm-source.exp) for aarch64-linux (Ubuntu 22.04 and 20.04). > > Could you please take a look at it? I can do some testing with the aarch64 machines if that's helpful. > > FAIL: gdb.asm/asm-source.exp: f at main > FAIL: gdb.asm/asm-source.exp: n at main > FAIL: gdb.asm/asm-source.exp: next over macro > FAIL: gdb.asm/asm-source.exp: list > FAIL: gdb.asm/asm-source.exp: f in foo2 > FAIL: gdb.asm/asm-source.exp: n in foo2 > FAIL: gdb.asm/asm-source.exp: bt ALL in foo2 > FAIL: gdb.asm/asm-source.exp: bt 2 in foo2 > FAIL: gdb.asm/asm-source.exp: bt 3 in foo3 > FAIL: gdb.asm/asm-source.exp: finish from foo3 > FAIL: gdb.asm/asm-source.exp: info source asmsrc2.s > FAIL: gdb.asm/asm-source.exp: info line > FAIL: gdb.asm/asm-source.exp: next over foo3 > FAIL: gdb.asm/asm-source.exp: return from foo2 Can confirm I'm seeing the same set of failures on x86-64 gdb when using gas that includes this patch. Thanks, Andrew > > === gdb Summary === > > # of expected passes 15 > # of unexpected failures 14 > > On 12/9/22 10:52, Jan Beulich via Binutils wrote: >> The PR gas/16908 workaround aimed at uniformly reporting line numbers >> to reference macro invocation sites. As mentioned in a comment this may >> be desirable for small macros, but often isn't for larger ones. As a >> first step improve diagnostics to report both locations, while aiming at >> leaving generated debug info unaltered. >> >> Note that macro invocation context is lost for any diagnostics issued >> only after all input was processed (or more generally for any use of >> as_*_where(), as the functions can't know whether the passed in location >> is related to [part of] the present stack of locations). To maintain the >> intended workaround behavior for PR gas/16908, a new as_where() is >> introduced to "look through" macro invocations, while the existing >> as_where() is renamed (and used in only very few places for now). Down >> the road as_where() will likely want to return a list of (file,line) >> pairs. >> --- >> v2: Fully cover Arm testsuite. >> --- >> Omitting testsuite adjustments from the inline patch, for its sheer size; >> please see the attachment for the full (compressed) patch. >> --- >> Adding >> >> if (subseg_text_p (now_seg)) >> dwarf2_emit_insn (0); >> >> to try_macro() (alongside using as_where_top() in dwarf2_where()) would >> get .debug_line contents into reasonable shape (expressing both macro >> invocation site and macro definition location). But that's likely >> insufficient: We may want / need to represent macros as inline >> subprograms and their expansions as inlined subroutines. Which in turn >> looks cumbersome especially with the relatively recently added recording >> of functions (with which macro expansions then would better be >> associated, when marked as such). >> >> --- a/gas/as.h >> +++ b/gas/as.h >> @@ -437,6 +437,10 @@ typedef struct _pseudo_type pseudo_typeS >> #define PRINTF_WHERE_LIKE(FCN) \ >> void FCN (const char *file, unsigned int line, const char *format, ...) \ >> __attribute__ ((__format__ (__printf__, 3, 4))) >> +#define PRINTF_INDENT_LIKE(FCN) \ >> + void FCN (const char *file, unsigned int line, unsigned int indent, \ >> + const char *format, ...) \ >> + __attribute__ ((__format__ (__printf__, 4, 5))) >> >> #else /* __GNUC__ < 2 || defined(VMS) */ >> >> @@ -444,6 +448,10 @@ typedef struct _pseudo_type pseudo_typeS >> #define PRINTF_WHERE_LIKE(FCN) void FCN (const char *file, \ >> unsigned int line, \ >> const char *format, ...) >> +#define PRINTF_INDENT_LIKE(FCN) void FCN (const char *file, \ >> + unsigned int line, \ >> + unsigned int indent, \ >> + const char *format, ...) >> >> #endif /* __GNUC__ < 2 || defined(VMS) */ >> >> @@ -453,6 +461,7 @@ PRINTF_LIKE (as_tsktsk); >> PRINTF_LIKE (as_warn); >> PRINTF_WHERE_LIKE (as_bad_where); >> PRINTF_WHERE_LIKE (as_warn_where); >> +PRINTF_INDENT_LIKE (as_info_where); >> >> void as_abort (const char *, int, const char *) ATTRIBUTE_NORETURN; >> void signal_init (void); >> @@ -487,7 +496,9 @@ void cond_finish_check (int); >> void cond_exit_macro (int); >> int seen_at_least_1_file (void); >> void app_pop (char *); >> +void as_report_context (void); >> const char * as_where (unsigned int *); >> +const char * as_where_top (unsigned int *); >> const char * as_where_physical (unsigned int *); >> void bump_line_counters (void); >> void do_scrub_begin (int); >> --- a/gas/input-scrub.c >> +++ b/gas/input-scrub.c >> @@ -104,6 +104,9 @@ static const char *logical_input_file; >> static unsigned int physical_input_line; >> static unsigned int logical_input_line; >> >> +/* Indicator whether the origin of an update was a .linefile directive. */ >> +static bool is_linefile; >> + >> /* Struct used to save the state of the input handler during include files */ >> struct input_save { >> char * buffer_start; >> @@ -115,6 +118,7 @@ struct input_save { >> const char * logical_input_file; >> unsigned int physical_input_line; >> unsigned int logical_input_line; >> + bool is_linefile; >> size_t sb_index; >> sb from_sb; >> enum expansion from_sb_expansion; /* Should we do a conditional check? */ >> @@ -166,6 +170,7 @@ input_scrub_push (char *saved_position) >> saved->logical_input_file = logical_input_file; >> saved->physical_input_line = physical_input_line; >> saved->logical_input_line = logical_input_line; >> + saved->is_linefile = is_linefile; >> saved->sb_index = sb_index; >> saved->from_sb = from_sb; >> saved->from_sb_expansion = from_sb_expansion; >> @@ -193,6 +198,7 @@ input_scrub_pop (struct input_save *save >> logical_input_file = saved->logical_input_file; >> physical_input_line = saved->physical_input_line; >> logical_input_line = saved->logical_input_line; >> + is_linefile = saved->is_linefile; >> sb_index = saved->sb_index; >> from_sb = saved->from_sb; >> from_sb_expansion = saved->from_sb_expansion; >> @@ -267,8 +273,6 @@ input_scrub_include_sb (sb *from, char * >> as_fatal (_("macros nested too deeply")); >> ++macro_nest; >> >> - gas_assert (expansion < expanding_nested); >> - >> #ifdef md_macro_start >> if (expansion == expanding_macro) >> { >> @@ -283,8 +287,6 @@ input_scrub_include_sb (sb *from, char * >> expansion. */ >> newline = from->len >= 1 && from->ptr[0] != '\n'; >> sb_build (&from_sb, from->len + newline + 2 * sizeof (".linefile") + 30); >> - if (expansion == expanding_repeat && from_sb_expansion >= expanding_macro) >> - expansion = expanding_nested; >> from_sb_expansion = expansion; >> if (newline) >> { >> @@ -437,10 +439,7 @@ bump_line_counters (void) >> if (sb_index == (size_t) -1) >> ++physical_input_line; >> >> - /* PR gas/16908 workaround: Don't bump logical line numbers while >> - expanding macros, unless file (and maybe line; see as_where()) are >> - used inside the macro. */ >> - if (logical_input_line != -1u && from_sb_expansion < expanding_macro) >> + if (logical_input_line != -1u) >> ++logical_input_line; >> } >> >> @@ -471,10 +470,6 @@ new_logical_line_flags (const char *fnam >> case 1 << 3: >> if (line_number < 0 || fname != NULL) >> abort (); >> - /* PR gas/16908 workaround: Ignore updates when nested inside a macro >> - expansion. */ >> - if (from_sb_expansion == expanding_nested) >> - return; >> if (next_saved_file == NULL) >> fname = physical_input_file; >> else if (next_saved_file->logical_input_file) >> @@ -486,6 +481,8 @@ new_logical_line_flags (const char *fnam >> abort (); >> } >> >> + is_linefile = flags != 1 && (flags != 0 || fname); >> + >> if (line_number >= 0) >> logical_input_line = line_number; >> else if (line_number == -1 && fname && !*fname && (flags & (1 << 2))) >> @@ -499,15 +496,6 @@ new_logical_line_flags (const char *fnam >> && (logical_input_file == NULL >> || filename_cmp (logical_input_file, fname))) >> logical_input_file = fname; >> - >> - /* When encountering file or line changes inside a macro, arrange for >> - bump_line_counters() to henceforth increment the logical line number >> - again, just like it does when expanding repeats. See as_where() for >> - why changing file or line alone doesn't alter expansion mode. */ >> - if (from_sb_expansion == expanding_macro >> - && logical_input_file != NULL >> - && logical_input_line != -1u) >> - from_sb_expansion = expanding_repeat; >> } >> >> void >> @@ -516,6 +504,33 @@ new_logical_line (const char *fname, int >> new_logical_line_flags (fname, line_number, 0); >> } >> >> +void >> +as_report_context (void) >> +{ >> + const struct input_save *saved = next_saved_file; >> + enum expansion expansion = from_sb_expansion; >> + int indent = 1; >> + >> + if (!macro_nest) >> + return; >> + >> + do >> + { >> + if (expansion != expanding_macro) >> + /* Nothing. */; >> + else if (saved->logical_input_file != NULL >> + && saved->logical_input_line != -1u) >> + as_info_where (saved->logical_input_file, saved->logical_input_line, >> + indent, _("macro invoked from here")); >> + else >> + as_info_where (saved->physical_input_file, saved->physical_input_line, >> + indent, _("macro invoked from here")); >> + >> + expansion = saved->from_sb_expansion; >> + ++indent; >> + } >> + while ((saved = saved->next_saved_file) != NULL); >> +} >> >> /* Return the current physical input file name and line number, if known */ >> >> @@ -534,11 +549,53 @@ as_where_physical (unsigned int *linep) >> return NULL; >> } >> >> -/* Return the current file name and line number. */ >> +/* Return the file name and line number at the top most macro >> + invocation, unless .file / .line were used inside a macro. */ >> >> const char * >> as_where (unsigned int *linep) >> { >> + const char *file = as_where_top (linep); >> + >> + if (macro_nest && is_linefile) >> + { >> + const struct input_save *saved = next_saved_file; >> + enum expansion expansion = from_sb_expansion; >> + >> + do >> + { >> + if (!saved->is_linefile) >> + break; >> + >> + if (expansion != expanding_macro) >> + /* Nothing. */; >> + else if (saved->logical_input_file != NULL >> + && (linep == NULL || saved->logical_input_line != -1u)) >> + { >> + if (linep != NULL) >> + *linep = saved->logical_input_line; >> + file = saved->logical_input_file; >> + } >> + else if (saved->physical_input_file != NULL) >> + { >> + if (linep != NULL) >> + *linep = saved->physical_input_line; >> + file = saved->physical_input_file; >> + } >> + >> + expansion = saved->from_sb_expansion; >> + } >> + while ((saved = saved->next_saved_file) != NULL); >> + } >> + >> + return file; >> +} >> + >> +/* Return the current file name and line number. */ >> + >> +const char * >> +as_where_top (unsigned int *linep) >> +{ >> if (logical_input_file != NULL >> && (linep == NULL || logical_input_line != -1u)) >> { >> @@ -549,4 +606,3 @@ as_where (unsigned int *linep) >> >> return as_where_physical (linep); >> } >> - >> --- a/gas/macro.c >> +++ b/gas/macro.c >> @@ -131,23 +131,21 @@ buffer_and_nest (const char *from, const >> else >> from_len = strlen (from); >> >> - /* Except for macros record the present source position, such that >> - diagnostics and debug info will be properly associated with the >> - respective original lines, rather than with the line of the ending >> - directive (TO). */ >> - if (from == NULL || strcasecmp (from, "MACRO") != 0) >> - { >> - unsigned int line; >> - char *linefile; >> - >> - as_where (&line); >> - if (!flag_m68k_mri) >> - linefile = xasprintf ("\t.linefile %u .", line + 1); >> - else >> - linefile = xasprintf ("\tlinefile %u .", line + 1); >> - sb_add_string (ptr, linefile); >> - xfree (linefile); >> - } >> + /* Record the present source position, such that diagnostics and debug info >> + can be properly associated with the respective original lines, rather >> + than with the line of the ending directive (TO). */ >> + { >> + unsigned int line; >> + char *linefile; >> + >> + as_where_top (&line); >> + if (!flag_m68k_mri) >> + linefile = xasprintf ("\t.linefile %u .", line + 1); >> + else >> + linefile = xasprintf ("\tlinefile %u .", line + 1); >> + sb_add_string (ptr, linefile); >> + xfree (linefile); >> + } >> >> while (more) >> { >> @@ -249,14 +247,8 @@ buffer_and_nest (const char *from, const >> } >> >> /* PR gas/16908 >> - Apply and discard .linefile directives that appear within >> - the macro. For long macros, one might want to report the >> - line number information associated with the lines within >> - the macro definition, but we would need more infrastructure >> - to make that happen correctly (e.g. resetting the line >> - number when expanding the macro), and since for short >> - macros we clearly prefer reporting the point of expansion >> - anyway, there's not an obviously better fix here. */ >> + Apply .linefile directives that appear within the macro, alongside >> + keeping them for later expansion of the macro. */ >> if (from != NULL && strcasecmp (from, "MACRO") == 0 >> && len >= 8 && strncasecmp (ptr->ptr + i, "linefile", 8) == 0) >> { >> @@ -267,7 +259,6 @@ buffer_and_nest (const char *from, const >> s_linefile (0); >> restore_ilp (); >> ptr->ptr[ptr->len] = saved_eol_char; >> - ptr->len = line_start; >> } >> } >> >> --- a/gas/messages.c >> +++ b/gas/messages.c >> @@ -18,6 +18,7 @@ >> 02110-1301, USA. */ >> >> #include "as.h" >> +#include >> #include >> >> /* If the system doesn't provide strsignal, we get it defined in >> @@ -119,7 +120,7 @@ as_show_where (void) >> const char *file; >> unsigned int line; >> >> - file = as_where (&line); >> + file = as_where_top (&line); >> identify (file); >> if (file) >> { >> @@ -130,6 +131,25 @@ as_show_where (void) >> } >> } >> >> +/* Send to stderr a string as information, with location data passed in. >> + Note that for now this is not intended for general use. */ >> + >> +void >> +as_info_where (const char *file, unsigned int line, unsigned int indent, >> + const char *format, ...) >> +{ >> + va_list args; >> + char buffer[2000]; >> + >> + gas_assert (file != NULL && line > 0 && indent <= INT_MAX); >> + >> + va_start (args, format); >> + vsnprintf (buffer, sizeof (buffer), format, args); >> + va_end (args); >> + fprintf (stderr, "%s:%u: %*s%s%s\n", >> + file, line, (int)indent, "", _("Info: "), buffer); >> +} >> + >> /* Send to stderr a string as a warning, and locate warning >> in input file(s). >> Please only use this for when we have some recovery action. >> @@ -146,6 +166,7 @@ as_tsktsk (const char *format, ...) >> vfprintf (stderr, format, args); >> va_end (args); >> (void) putc ('\n', stderr); >> + as_report_context (); >> } >> >> /* The common portion of as_warn and as_warn_where. */ >> @@ -153,10 +174,15 @@ as_tsktsk (const char *format, ...) >> static void >> as_warn_internal (const char *file, unsigned int line, char *buffer) >> { >> + bool context = false; >> + >> ++warning_count; >> >> if (file == NULL) >> - file = as_where (&line); >> + { >> + file = as_where_top (&line); >> + context = true; >> + } >> >> identify (file); >> if (file) >> @@ -168,6 +194,10 @@ as_warn_internal (const char *file, unsi >> } >> else >> fprintf (stderr, "%s%s\n", _("Warning: "), buffer); >> + >> + if (context) >> + as_report_context (); >> + >> #ifndef NO_LISTING >> listing_warning (buffer); >> #endif >> @@ -194,7 +224,7 @@ as_warn (const char *format, ...) >> } >> } >> >> -/* Like as_bad but the file name and line number are passed in. >> +/* Like as_warn but the file name and line number are passed in. >> Unfortunately, we have to repeat the function in order to handle >> the varargs correctly and portably. */ >> >> @@ -218,10 +248,15 @@ as_warn_where (const char *file, unsigne >> static void >> as_bad_internal (const char *file, unsigned int line, char *buffer) >> { >> + bool context = false; >> + >> ++error_count; >> >> if (file == NULL) >> - file = as_where (&line); >> + { >> + file = as_where_top (&line); >> + context = true; >> + } >> >> identify (file); >> if (file) >> @@ -233,6 +268,10 @@ as_bad_internal (const char *file, unsig >> } >> else >> fprintf (stderr, "%s%s\n", _("Error: "), buffer); >> + >> + if (context) >> + as_report_context (); >> + >> #ifndef NO_LISTING >> listing_error (buffer); >> #endif >> @@ -290,6 +329,7 @@ as_fatal (const char *format, ...) >> vfprintf (stderr, format, args); >> (void) putc ('\n', stderr); >> va_end (args); >> + as_report_context (); >> /* Delete the output file, if it exists. This will prevent make from >> thinking that a file was created and hence does not need rebuilding. */ >> if (out_file_name != NULL) >> @@ -312,6 +352,7 @@ as_abort (const char *file, int line, co >> fprintf (stderr, _("Internal error in %s at %s:%d.\n"), fn, file, line); >> else >> fprintf (stderr, _("Internal error at %s:%d.\n"), file, line); >> + as_report_context (); >> >> fprintf (stderr, _("Please report this bug.\n")); >> >> --- a/gas/sb.h >> +++ b/gas/sb.h >> @@ -66,11 +66,9 @@ extern size_t sb_skip_comma (size_t, sb >> >> /* Actually in input-scrub.c. */ >> enum expansion { >> - /* Note: Order matters! */ >> expanding_none, >> expanding_repeat, >> expanding_macro, >> - expanding_nested, /* Only for internal use of input-scrub.c. */ >> }; >> extern void input_scrub_include_sb (sb *, char *, enum expansion); >>