[PATCH] gas/symbols: do not panic upon resolving O_md

[PATCH] gas/symbols: do not panic upon resolving O_md

[Dmitry Selyutin]

Assuming GMSD is a special operand, marked as O_md1, the code:

    .set VREG, GMSD
    .set REG, VREG
    extsw REG, 2

...fails upon attempts to resolve the value of the symbol. This happens
since machine-dependent values are not handled in the giant op switch.

Machine-dependent expressions don't really need to be resolved, since
the resolving process is really machine-dependent. We could have marked
such symbols as resolving in port. However, we don't want to access
the field which seems to be internal, and we especially don't want
to perform this for each and every port.
---
 gas/symbols.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/gas/symbols.c b/gas/symbols.c
index 6904a3102c..fd2117f7da 100644
--- a/gas/symbols.c
+++ b/gas/symbols.c
@@ -1408,6 +1408,16 @@ resolve_symbol_value (symbolS *symp)
 	  BAD_CASE (op);
 	  break;
 
+	case O_md1...O_md32:
+	  /* Machine-dependent expressions don't really need
+	   * to be resolved, since the resolving process is
+	   * really machine-dependent. We could have marked
+	   * such symbols as resolving in port. However, we
+	   * don't want to access the field which seems to be
+	   * internal, and we especially don't want to perform
+	   * this for each and every port. */
+	  break;
+
 	case O_absent:
 	  final_val = 0;
 	  /* Fall through.  */
-- 
2.37.0

Re: [PATCH] gas/symbols: do not panic upon resolving O_md

[Alan Modra]

On Fri, Jul 15, 2022 at 12:26:52AM +0300, Dmitry Selyutin wrote:
> Assuming GMSD is a special operand, marked as O_md1, the code:
> 
>     .set VREG, GMSD
>     .set REG, VREG
>     extsw REG, 2
> 
> ...fails upon attempts to resolve the value of the symbol. This happens
> since machine-dependent values are not handled in the giant op switch.
> 
> Machine-dependent expressions don't really need to be resolved, since
> the resolving process is really machine-dependent. We could have marked
> such symbols as resolving in port. However, we don't want to access
> the field which seems to be internal, and we especially don't want
> to perform this for each and every port.
> ---
>  gas/symbols.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/gas/symbols.c b/gas/symbols.c
> index 6904a3102c..fd2117f7da 100644
> --- a/gas/symbols.c
> +++ b/gas/symbols.c
> @@ -1408,6 +1408,16 @@ resolve_symbol_value (symbolS *symp)
>  	  BAD_CASE (op);
>  	  break;
>  
> +	case O_md1...O_md32:

Please don't use gcc extensions.

> +	  /* Machine-dependent expressions don't really need
> +	   * to be resolved, since the resolving process is
> +	   * really machine-dependent. We could have marked
> +	   * such symbols as resolving in port. However, we
> +	   * don't want to access the field which seems to be
> +	   * internal, and we especially don't want to perform
> +	   * this for each and every port. */
> +	  break;

This might avoid the fatal error, but will still hit an error later
due to resolved not being set.  I think this calls for an
md_resolve_symbol.

> +
>  	case O_absent:
>  	  final_val = 0;
>  	  /* Fall through.  */
> -- 
> 2.37.0

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] gas/symbols: do not panic upon resolving O_md

[Dmitry Selyutin]

On Fri, Jul 15, 2022, 07:50 Alan Modra <amodra@gmail.com> wrote:

> On Fri, Jul 15, 2022 at 12:26:52AM +0300, Dmitry Selyutin wrote:
> > +     case O_md1...O_md32:
>
> Please don't use gcc extensions.
>

My initial impression was that the code uses them (e.g. ATTRIBUTE_UNUSED).
But OK, actually the first thing I did was a simple "if" in the default
section; I only thought that ranges would be more evident. Alternatively,
we could have a bunch of O_md cases; 32 cases with just a simple break
might be way too much, though. With md_resolve_symbol this makes more
sense, I think.

This might avoid the fatal error, but will still hit an error later
> due to resolved not being set.


I assume you mean symbol->flags.resolving = 0, right? If so, this is set
after the switch (including this break).

I think this calls for an
> md_resolve_symbol.
>

Do you mean letting the caller define this as macro? I suggest calling it
in "default:" section then, with the corresponding ifdef check.

Re: [PATCH] gas/symbols: do not panic upon resolving O_md

[Alan Modra]

On Fri, Jul 15, 2022 at 08:13:01AM +0300, Dmitry Selyutin wrote:
> On Fri, Jul 15, 2022, 07:50 Alan Modra <amodra@gmail.com> wrote:
> 
> > On Fri, Jul 15, 2022 at 12:26:52AM +0300, Dmitry Selyutin wrote:
> > > +     case O_md1...O_md32:
> >
> > Please don't use gcc extensions.
> >
> 
> My initial impression was that the code uses them (e.g. ATTRIBUTE_UNUSED).
> But OK, actually the first thing I did was a simple "if" in the default
> section; I only thought that ranges would be more evident. Alternatively,
> we could have a bunch of O_md cases; 32 cases with just a simple break
> might be way too much, though. With md_resolve_symbol this makes more
> sense, I think.
> 
> > This might avoid the fatal error, but will still hit an error later
> > due to resolved not being set.
> 
> 
> I assume you mean symbol->flags.resolving = 0, right? If so, this is set
> after the switch (including this break).

No, I mean what I said.

      if (resolved)
	symp->flags.resolved = 1;
      else if (S_GET_SEGMENT (symp) != expr_section)
	{
	  as_bad (_("can't resolve value for symbol `%s'"),
		  S_GET_NAME (symp));
	  symp->flags.resolved = 1;
	}

> I think this calls for an
> > md_resolve_symbol.
> >
> 
> Do you mean letting the caller define this as macro? I suggest calling it
> in "default:" section then, with the corresponding ifdef check.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] gas/symbols: do not panic upon resolving O_md

[Dmitry Selyutin]

On Sat, Jul 16, 2022 at 5:02 AM Alan Modra <amodra@gmail.com> wrote:
> > > This might avoid the fatal error, but will still hit an error later
> > > due to resolved not being set.
> > I assume you mean symbol->flags.resolving = 0, right? If so, this is set
> > after the switch (including this break).
> No, I mean what I said.
>
>       if (resolved)
>         symp->flags.resolved = 1;
>       else if (S_GET_SEGMENT (symp) != expr_section)
>         {
>           as_bad (_("can't resolve value for symbol `%s'"),
>                   S_GET_NAME (symp));
>           symp->flags.resolved = 1;
>         }

Hi Alan, could you, please, provide more details? Assuming the code
below (note the asterisk which SVP64 uses to mark the vector
registers)...

.set VREG, *%r5
.set REG, VREG
extsw REG, 2

...I end up with the following:

symp->x->value->X_op = O_md1
final_seg == expr_section

So yes, the symbol is not marked as resolved; but it passes the check
since it's in the expr section.

> > > I think this calls for an
> > > md_resolve_symbol.
> > Do you mean letting the caller define this as macro? I suggest calling it
> > in "default:" section then, with the corresponding ifdef check.

I've entered a bunch of cases instead. Looks big but at least explicit enough.

-- 
Best regards,
Dmitry Selyutin

[PATCH] gas/symbols: introduce md_resolve_symbol

[Dmitry Selyutin]

Assuming GMSD is a special operand, marked as O_md1, the code:

    .set VREG, GMSD
    .set REG, VREG
    extsw REG, 2

...fails upon attempts to resolve the value of the symbol. This happens
since machine-dependent values are not handled in the giant op switch.
We introduce a custom md_resolve_symbol macro; the ports can use this
macro to customize the behavior when resolve_symbol_value hits O_md
operand.
---
 gas/doc/internals.texi |  6 ++++++
 gas/symbols.c          | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/gas/doc/internals.texi b/gas/doc/internals.texi
index 9221801f63..59b2d3acee 100644
--- a/gas/doc/internals.texi
+++ b/gas/doc/internals.texi
@@ -1034,6 +1034,12 @@ Predefined symbols with fixed values, such as register names or condition
 codes, are typically entered directly into the symbol table when @code{md_begin}
 is called.  One argument is passed, a @code{char *} for the symbol.
 
+@item md_resolve_symbol
+@cindex md_resolve_symbol
+If this macro is defined, GAS will call it upon resolving machine-dependent
+symbols (that is, for any symbol with operation O_md1..O_md32 inclusively).
+If this functions returns zero, then the symbol could not be resolved.
+
 @item md_operand
 @cindex md_operand
 GAS will call this function with one argument, an @code{expressionS}
diff --git a/gas/symbols.c b/gas/symbols.c
index 6904a3102c..637ecd7515 100644
--- a/gas/symbols.c
+++ b/gas/symbols.c
@@ -1408,6 +1408,44 @@ resolve_symbol_value (symbolS *symp)
 	  BAD_CASE (op);
 	  break;
 
+	case O_md1:
+	case O_md2:
+	case O_md3:
+	case O_md4:
+	case O_md5:
+	case O_md6:
+	case O_md7:
+	case O_md8:
+	case O_md9:
+	case O_md10:
+	case O_md11:
+	case O_md12:
+	case O_md13:
+	case O_md14:
+	case O_md15:
+	case O_md16:
+	case O_md17:
+	case O_md18:
+	case O_md19:
+	case O_md20:
+	case O_md21:
+	case O_md22:
+	case O_md23:
+	case O_md24:
+	case O_md25:
+	case O_md26:
+	case O_md27:
+	case O_md28:
+	case O_md29:
+	case O_md30:
+	case O_md31:
+	case O_md32:
+#ifdef md_resolve_symbol
+	  if (md_resolve_symbol (symp, &final_val))
+	    break;
+#endif
+	  goto exit_dont_set_value;
+
 	case O_absent:
 	  final_val = 0;
 	  /* Fall through.  */
-- 
2.37.0

Re: [PATCH] gas/symbols: introduce md_resolve_symbol

[Alan Modra]

On Mon, Jul 18, 2022 at 12:46:53PM +0300, Dmitry Selyutin wrote:
> Assuming GMSD is a special operand, marked as O_md1, the code:
> 
>     .set VREG, GMSD
>     .set REG, VREG
>     extsw REG, 2
> 
> ...fails upon attempts to resolve the value of the symbol. This happens
> since machine-dependent values are not handled in the giant op switch.
> We introduce a custom md_resolve_symbol macro; the ports can use this
> macro to customize the behavior when resolve_symbol_value hits O_md
> operand.

Applied to mainline with a small tweak.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] gas/symbols: introduce md_resolve_symbol

[Dmitry Selyutin]

On Wed, Jul 20, 2022 at 6:06 AM Alan Modra <amodra@gmail.com> wrote:
> Applied to mainline with a small tweak.

Thanks for tweaking this! It appears I could not apply O_md1
(O_vector, used to mark vector registers) correctly unless I also tune
the segment appropriately.

    .set VREG, *%r11
    sv.extsw VREG, 2
    as-new: unable to find equivalent output section for symbol 'VREG'
from section '*GAS `expr' section*'

I tuned the section so that the O_md1 is considered to belong to a
register section, but I'm not sure if I did it right and if this is
not a hack.
https://git.libre-soc.org/?p=binutils-gdb.git;a=commit;h=9ce9a1af040b22c844fa5312d583e6d30cd5dadb

Alan, could you, please, take a look? The main changes are at tc-ppc.c.

-- 
Best regards,
Dmitry Selyutin