Re: [PATCH, V2 06/10] gas: scfidw2gen: new functionality to prepapre for SCFI

[Jan Beulich <jbeulich@suse.com>]

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?

> +   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).

> +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?

> +  {
> +    { "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?

> +    { "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/ ?

> +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.

> +#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?

> --- /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).

Jan