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

[Indu Bhagat]

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.