From: Indu Bhagat <indu.bhagat@oracle.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: binutils@sourceware.org
Subject: Re: [PATCH, V2 06/10] gas: scfidw2gen: new functionality to prepapre for SCFI
Date: Tue, 31 Oct 2023 15:06:19 -0700 [thread overview]
Message-ID: <7dafd71a-33cc-4894-bec0-9c6621cb8144@oracle.com> (raw)
In-Reply-To: <ee060731-68b3-494b-01dc-49431fbed2dd@suse.com>
Hi Jan,
Thanks for reviewing.
On 10/31/23 04:28, Jan Beulich wrote:
> On 30.10.2023 17:51, Indu Bhagat wrote:
>> --- /dev/null
>> +++ b/gas/scfidw2gen.c
>> @@ -0,0 +1,305 @@
>> +/* scfidw2gen.c - Support for emission of synthesized Dwarf2 CFI.
>> + Copyright (C) 2003-2023 Free Software Foundation, Inc.
>
> Is this year range really applicable to this new file?
>
No. I will fix it here and other files.
>> + This file is part of GAS, the GNU Assembler.
>> +
>> + GAS is free software; you can redistribute it and/or modify
>> + it under the terms of the GNU General Public License as published by
>> + the Free Software Foundation; either version 3, or (at your option)
>> + any later version.
>> +
>> + GAS is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + GNU General Public License for more details.
>> +
>> + You should have received a copy of the GNU General Public License
>> + along with GAS; see the file COPYING. If not, write to the Free
>> + Software Foundation, 51 Franklin Street - Fifth Floor, Boston, MA
>> + 02110-1301, USA. */
>> +
>> +#include "as.h"
>> +#include "dw2gencfi.h"
>> +#include "subsegs.h"
>> +#include "scfidw2gen.h"
>> +
>> +#if defined (TARGET_USE_SCFI) && defined (TARGET_USE_GINSN)
>> +
>> +static int scfi_ignore_warn_once = 0;
>
> Nit: bool please (and no real need for an initializer).
>
OK.
>> +static void dot_scfi_sections (int);
>> +static void dot_scfi_ignore (int);
>> +static void dot_scfi (int);
>
> May I suggest to avoid such forward declarations by moving ...
>
>> +const pseudo_typeS scfi_pseudo_table[] =
>
> ... this table towards the bottom of the file?
>
OK.
>> + {
>> + { "cfi_sections", dot_scfi_sections, 0 }, /* No ignore. */
>
> Instead of three such individual comments, how about putting the three
> relevant ones first, followed by a comment (serving as a separator) and
> then all dot_scfi_ignore entries?
>
OK.
>> + { "cfi_startproc", dot_scfi_ignore, 0 },
>> + { "cfi_endproc", dot_scfi_ignore, 0 },
>> + { "cfi_fde_data", dot_scfi_ignore, 0 },
>> + { "cfi_def_cfa", dot_scfi_ignore, 0 },
>> + { "cfi_def_cfa_register", dot_scfi_ignore, 0 },
>> + { "cfi_def_cfa_offset", dot_scfi_ignore, 0 },
>> + { "cfi_adjust_cfa_offset", dot_scfi_ignore, 0 },
>> + { "cfi_offset", dot_scfi_ignore, 0 },
>> + { "cfi_rel_offset", dot_scfi_ignore, 0 },
>> + { "cfi_register", dot_scfi_ignore, 0 },
>> + { "cfi_return_column", dot_scfi_ignore, 0 },
>> + { "cfi_restore", dot_scfi_ignore, 0 },
>> + { "cfi_undefined", dot_scfi_ignore, 0 },
>> + { "cfi_same_value", dot_scfi_ignore, 0 },
>> + { "cfi_remember_state", dot_scfi_ignore, 0 },
>> + { "cfi_restore_state", dot_scfi_ignore, 0 },
>> + { "cfi_window_save", dot_scfi_ignore, 0 },
>> + { "cfi_negate_ra_state", dot_scfi_ignore, 0 },
>> + { "cfi_escape", dot_scfi_ignore, 0 },
>> + { "cfi_signal_frame", dot_scfi, CFI_signal_frame }, /* No ignore. */
>> + { "cfi_personality", dot_scfi_ignore, 0 },
>> + { "cfi_personality_id", dot_scfi_ignore, 0 },
>> + { "cfi_lsda", dot_scfi_ignore, 0 },
>> + { "cfi_val_encoded_addr", dot_scfi_ignore, 0 },
>> + { "cfi_inline_lsda", dot_scfi_ignore, 0 },
>> + { "cfi_label", dot_scfi, CFI_label }, /* No ignore. */
>> + { "cfi_val_offset", dot_scfi_ignore, 0 },
>> + { NULL, NULL, 0 }
>> + };
>> +
>> +static void
>> +dot_scfi_ignore (int ignored ATTRIBUTE_UNUSED)
>> +{
>> + gas_assert (flag_synth_cfi);
>> +
>> + if (scfi_ignore_warn_once == 0)
>> + {
>> + as_warn (_("--scfi=all ignores some user-specified CFI directives"));
>
> s/some/most/ ?
>
Hmm. Technically, "most" is the correct choice. So, OK.
>> +void
>> +scfi_dot_cfi_startproc (symbolS *start_sym)
>
> This and the following two functions presently have no caller, and I
> also can't spot equivalents in dw2gencfi.c. How are they (going) to be
> used? This ...
>
>> +{
>> + if (frchain_now->frch_cfi_data != NULL)
>> + {
>> + as_bad (_("previous CFI entry not closed (missing .cfi_endproc)"));
>
> ... for example suggests that the function here might really be the
> handler for .cfi_startproc, yet the table above says .cfi_startproc is
> ignored.
>
The callers of scfi_dot_cfi_startproc (), scfi_dot_cfi_endproc () and
scfi_dot_cfi () are in scfi.c, when its time to emit DWARF CFI after
SCFI machinery has generated the SCFI Ops (See scfi_dot_cfi () and
scfi_emit_dw2cfi ()). The callers are added in the next patch in the
series, "[PATCH, V2 07/10] gas: synthesize CFI for hand-written asm".
>> +#else
>> +
>> +static void
>> +dot_scfi_dummy (int ignored ATTRIBUTE_UNUSED)
>> +{
>> + as_bad (_("SCFI is not supported for this target"));
>> + ignore_rest_of_line ();
>> +}
>> +
>> +const pseudo_typeS scfi_pseudo_table[] =
>> + {
>> + { "cfi_sections", dot_scfi_dummy, 0 },
>> + { "cfi_startproc", dot_scfi_dummy, 0 },
>> + { "cfi_endproc", dot_scfi_dummy, 0 },
>> + { "cfi_fde_data", dot_scfi_dummy, 0 },
>> + { "cfi_def_cfa", dot_scfi_dummy, 0 },
>> + { "cfi_def_cfa_register", dot_scfi_dummy, 0 },
>> + { "cfi_def_cfa_offset", dot_scfi_dummy, 0 },
>> + { "cfi_adjust_cfa_offset", dot_scfi_dummy, 0 },
>> + { "cfi_offset", dot_scfi_dummy, 0 },
>> + { "cfi_rel_offset", dot_scfi_dummy, 0 },
>> + { "cfi_register", dot_scfi_dummy, 0 },
>> + { "cfi_return_column", dot_scfi_dummy, 0 },
>> + { "cfi_restore", dot_scfi_dummy, 0 },
>> + { "cfi_undefined", dot_scfi_dummy, 0 },
>> + { "cfi_same_value", dot_scfi_dummy, 0 },
>> + { "cfi_remember_state", dot_scfi_dummy, 0 },
>> + { "cfi_restore_state", dot_scfi_dummy, 0 },
>> + { "cfi_window_save", dot_scfi_dummy, 0 },
>> + { "cfi_negate_ra_state", dot_scfi_dummy, 0 },
>> + { "cfi_escape", dot_scfi_dummy, 0 },
>> + { "cfi_signal_frame", dot_scfi_dummy, 0 },
>> + { "cfi_personality", dot_scfi_dummy, 0 },
>> + { "cfi_personality_id", dot_scfi_dummy, 0 },
>> + { "cfi_lsda", dot_scfi_dummy, 0 },
>> + { "cfi_val_encoded_addr", dot_scfi_dummy, 0 },
>> + { "cfi_inline_lsda", dot_scfi_dummy, 0 },
>> + { "cfi_label", dot_scfi_dummy, 0 },
>> + { "cfi_val_offset", dot_scfi_dummy, 0 },
>> + { NULL, NULL, 0 }
>> + };
>> +
>> +#endif
>
> Is this really needed? Can't you simply error on use of the command
> line option, without the need for the extra table and dummy handler?
>
Actually, gas does not even present the option --scfi when the target
does not have the two required defines TARGET_USE_SCFI and
TARGET_USE_GINSN. So I could guard the code in pobegin () in read.c
(and remove the dummy handlers in scfidw2genc.c):
/* Now CFI ones. */
#if defined (TARGET_USE_SCFI) && defined (TARGET_USE_GINSN)
if (flag_synth_cfi)
{
pop_table_name = "scfi";
scfi_pop_insert ();
}
else
{
pop_table_name = "cfi";
cfi_pop_insert ();
}
#else
pop_table_name = "cfi";
cfi_pop_insert ();
#endif
>> --- /dev/null
>> +++ b/gas/scfidw2gen.h
>> @@ -0,0 +1,37 @@
>> +/* scfidw2gen.h - Support for emitting synthesized Dwarf2 CFI.
>> + Copyright (C) 2003-2023 Free Software Foundation, Inc.
>> +
>> + This file is part of GAS, the GNU Assembler.
>> +
>> + GAS is free software; you can redistribute it and/or modify
>> + it under the terms of the GNU General Public License as published by
>> + the Free Software Foundation; either version 3, or (at your option)
>> + any later version.
>> +
>> + GAS is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + GNU General Public License for more details.
>> +
>> + You should have received a copy of the GNU General Public License
>> + along with GAS; see the file COPYING. If not, write to the Free
>> + Software Foundation, 51 Franklin Street - Fifth Floor, Boston, MA
>> + 02110-1301, USA. */
>> +
>> +#ifndef SCFIDW2GEN_H
>> +#define SCFIDW2GEN_H
>> +
>> +#include "as.h"
>> +#include "dwarf2.h"
>> +
>> +extern int all_cfi_sections;
>
> This needs to go into dw2gencfi.h, such that dw2gencfi.c will also see
> the declaration (and the compiler be able to check that declaration and
> definition are in sync).
>
OK.
next prev parent reply other threads:[~2023-10-31 22:06 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-30 16:51 [PATCH, V2 00/10] Synthesize CFI for hand-written asm Indu Bhagat
2023-10-30 16:51 ` [PATCH, V2 01/10] gas: dw2gencfi: minor rejig for cfi_sections_set and all_cfi_sections Indu Bhagat
2023-10-30 16:51 ` [PATCH, V2 02/10] gas: dw2gencfi: use all_cfi_sections instead of cfi_sections Indu Bhagat
2023-10-30 16:51 ` [PATCH, V2 03/10] gas: dw2gencfi: expose a new cfi_set_last_fde API Indu Bhagat
2023-10-30 16:51 ` [PATCH, V2 04/10] gas: dw2gencfi: move some tc_* defines to the header file Indu Bhagat
2023-10-30 16:51 ` [PATCH, V2 05/10] gas: add new command line option --scfi[=all,none] Indu Bhagat
2023-10-30 16:51 ` [PATCH, V2 06/10] gas: scfidw2gen: new functionality to prepapre for SCFI Indu Bhagat
2023-10-31 11:28 ` Jan Beulich
2023-10-31 22:06 ` Indu Bhagat [this message]
2023-11-02 10:35 ` Jan Beulich
2023-11-02 16:32 ` Indu Bhagat
2023-10-30 16:51 ` [PATCH, V2 07/10] gas: synthesize CFI for hand-written asm Indu Bhagat
2023-10-31 14:10 ` Jan Beulich
2023-11-02 8:15 ` Indu Bhagat
2023-11-02 11:39 ` Jan Beulich
2023-12-10 10:22 ` Indu Bhagat
2023-12-11 7:59 ` Jan Beulich
2023-12-19 7:07 ` Indu Bhagat
2023-11-02 15:53 ` Jan Beulich
2023-11-04 7:29 ` Indu Bhagat
2023-11-06 11:03 ` Jan Beulich
2023-12-10 10:23 ` Indu Bhagat
2023-12-11 8:02 ` Jan Beulich
2023-10-30 16:51 ` [PATCH, V2 08/10] gas: doc: update documentation for the new listing option Indu Bhagat
2023-10-30 16:51 ` [PATCH, V2 09/10] gas: testsuite: add a x86_64 testsuite for SCFI Indu Bhagat
2023-10-31 16:13 ` Jan Beulich
2023-11-01 6:24 ` Indu Bhagat
2023-11-02 12:28 ` Jan Beulich
2023-11-03 5:45 ` Indu Bhagat
2023-10-30 16:51 ` [PATCH, V2 10/10] gas/NEWS: announce the new command line option Indu Bhagat
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7dafd71a-33cc-4894-bec0-9c6621cb8144@oracle.com \
--to=indu.bhagat@oracle.com \
--cc=binutils@sourceware.org \
--cc=jbeulich@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).