diff mbox series

x86: adjust which Dwarf2 register numbers to use

Message ID 262f296e-673b-47f0-a764-276939161d64@suse.com
State Unresolved
Headers
Series x86: adjust which Dwarf2 register numbers to use |

Checks

Context Check Description
snail/binutils-gdb-check warning Git am fail log

Commit Message

Jan Beulich Feb. 9, 2024, 8:11 a.m. UTC 
  Consumers can't know which execution mode is in effect for a certain
piece of code; they can only go from object file properties. Hence which
register numbers to encode ought to depend solely on object file type.

Comments

Indu Bhagat Feb. 15, 2024, 10:22 p.m. UTC | #1 
On 2/9/24 00:11, Jan Beulich wrote:
> Consumers can't know which execution mode is in effect for a certain
> piece of code; they can only go from object file properties. Hence which
> register numbers to encode ought to depend solely on object file type.
> 
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -5409,7 +5409,7 @@ ginsn_dw2_regnum (const reg_entry *ireg)
>     if (ireg->reg_num == RegIP || ireg->reg_num == RegIZ)
>       return GINSN_DW2_REGNUM_RSI_DUMMY;
>   
> -  dwarf_reg = ireg->dw2_regnum[flag_code >> 1];
> +  dwarf_reg = ireg->dw2_regnum[object_64bit];
>   
>     if (dwarf_reg == Dw2Inval)
>       {
> @@ -17461,7 +17461,7 @@ tc_x86_parse_to_dw2regnum (expressionS *
>         if ((addressT) exp->X_add_number < i386_regtab_size)
>   	{
>   	  exp->X_add_number = i386_regtab[exp->X_add_number]
> -			      .dw2_regnum[flag_code >> 1];
> +			      .dw2_regnum[object_64bit];
>   	  if (exp->X_add_number != Dw2Inval)
>   	    exp->X_op = O_constant;
>   	}

On one hand, I see that the suggested code changes are making things 
semantically clearer, I would like to understand:

1. If there is a scenario where flag_code is CODE16_BIT / CODE32_BIT and 
object_64bit equal to 1 is supported.  gcc passes --32 when using -m16 
or -m32.

2. Irrespective of #1, shouldn't we then also use "if (object_64bit == 
1)" instead of "if (flag_code == CODE_64BIT)" in md_begin where we set 
the value of x86_dwarf2_return_column etc ?

Thanks
Jan Beulich Feb. 16, 2024, 7:26 a.m. UTC | #2 
On 15.02.2024 23:22, Indu Bhagat wrote:
> On 2/9/24 00:11, Jan Beulich wrote:
>> Consumers can't know which execution mode is in effect for a certain
>> piece of code; they can only go from object file properties. Hence which
>> register numbers to encode ought to depend solely on object file type.
>>
>> --- a/gas/config/tc-i386.c
>> +++ b/gas/config/tc-i386.c
>> @@ -5409,7 +5409,7 @@ ginsn_dw2_regnum (const reg_entry *ireg)
>>     if (ireg->reg_num == RegIP || ireg->reg_num == RegIZ)
>>       return GINSN_DW2_REGNUM_RSI_DUMMY;
>>   
>> -  dwarf_reg = ireg->dw2_regnum[flag_code >> 1];
>> +  dwarf_reg = ireg->dw2_regnum[object_64bit];
>>   
>>     if (dwarf_reg == Dw2Inval)
>>       {
>> @@ -17461,7 +17461,7 @@ tc_x86_parse_to_dw2regnum (expressionS *
>>         if ((addressT) exp->X_add_number < i386_regtab_size)
>>   	{
>>   	  exp->X_add_number = i386_regtab[exp->X_add_number]
>> -			      .dw2_regnum[flag_code >> 1];
>> +			      .dw2_regnum[object_64bit];
>>   	  if (exp->X_add_number != Dw2Inval)
>>   	    exp->X_op = O_constant;
>>   	}
> 
> On one hand, I see that the suggested code changes are making things 
> semantically clearer, I would like to understand:
> 
> 1. If there is a scenario where flag_code is CODE16_BIT / CODE32_BIT and 
> object_64bit equal to 1 is supported.  gcc passes --32 when using -m16 
> or -m32.

Well, gcc may never produce such input, but hand-written assembly can.

> 2. Irrespective of #1, shouldn't we then also use "if (object_64bit == 
> 1)" instead of "if (flag_code == CODE_64BIT)" in md_begin where we set 
> the value of x86_dwarf2_return_column etc ?

Good point, let me make a v2.

Jan
Indu Bhagat Feb. 20, 2024, 11:04 p.m. UTC | #3 
On 2/15/24 23:26, Jan Beulich wrote:
> On 15.02.2024 23:22, Indu Bhagat wrote:
>> On 2/9/24 00:11, Jan Beulich wrote:
>>> Consumers can't know which execution mode is in effect for a certain
>>> piece of code; they can only go from object file properties. Hence which
>>> register numbers to encode ought to depend solely on object file type.
>>>
>>> --- a/gas/config/tc-i386.c
>>> +++ b/gas/config/tc-i386.c
>>> @@ -5409,7 +5409,7 @@ ginsn_dw2_regnum (const reg_entry *ireg)
>>>      if (ireg->reg_num == RegIP || ireg->reg_num == RegIZ)
>>>        return GINSN_DW2_REGNUM_RSI_DUMMY;
>>>    
>>> -  dwarf_reg = ireg->dw2_regnum[flag_code >> 1];
>>> +  dwarf_reg = ireg->dw2_regnum[object_64bit];
>>>    
>>>      if (dwarf_reg == Dw2Inval)
>>>        {
>>> @@ -17461,7 +17461,7 @@ tc_x86_parse_to_dw2regnum (expressionS *
>>>          if ((addressT) exp->X_add_number < i386_regtab_size)
>>>    	{
>>>    	  exp->X_add_number = i386_regtab[exp->X_add_number]
>>> -			      .dw2_regnum[flag_code >> 1];
>>> +			      .dw2_regnum[object_64bit];
>>>    	  if (exp->X_add_number != Dw2Inval)
>>>    	    exp->X_op = O_constant;
>>>    	}
>>
>> On one hand, I see that the suggested code changes are making things
>> semantically clearer, I would like to understand:
>>
>> 1. If there is a scenario where flag_code is CODE16_BIT / CODE32_BIT and
>> object_64bit equal to 1 is supported.  gcc passes --32 when using -m16
>> or -m32.
> 
> Well, gcc may never produce such input, but hand-written assembly can.
> 

Then, should we also use sp[object_64bit] instead of sp[flag_code >> 1] 
in tc_x86_frame_initial_instructions? Otherwise the assert "gas_assert 
(exp.X_op == O_constant)" will trigger, e.g. with .code16 and --64.

>> 2. Irrespective of #1, shouldn't we then also use "if (object_64bit ==
>> 1)" instead of "if (flag_code == CODE_64BIT)" in md_begin where we set
>> the value of x86_dwarf2_return_column etc ?
> 
> Good point, let me make a v2.
> 
> Jan
Jan Beulich Feb. 21, 2024, 7:34 a.m. UTC | #4 
On 21.02.2024 00:04, Indu Bhagat wrote:
> On 2/15/24 23:26, Jan Beulich wrote:
>> On 15.02.2024 23:22, Indu Bhagat wrote:
>>> On 2/9/24 00:11, Jan Beulich wrote:
>>>> Consumers can't know which execution mode is in effect for a certain
>>>> piece of code; they can only go from object file properties. Hence which
>>>> register numbers to encode ought to depend solely on object file type.
>>>>
>>>> --- a/gas/config/tc-i386.c
>>>> +++ b/gas/config/tc-i386.c
>>>> @@ -5409,7 +5409,7 @@ ginsn_dw2_regnum (const reg_entry *ireg)
>>>>      if (ireg->reg_num == RegIP || ireg->reg_num == RegIZ)
>>>>        return GINSN_DW2_REGNUM_RSI_DUMMY;
>>>>    
>>>> -  dwarf_reg = ireg->dw2_regnum[flag_code >> 1];
>>>> +  dwarf_reg = ireg->dw2_regnum[object_64bit];
>>>>    
>>>>      if (dwarf_reg == Dw2Inval)
>>>>        {
>>>> @@ -17461,7 +17461,7 @@ tc_x86_parse_to_dw2regnum (expressionS *
>>>>          if ((addressT) exp->X_add_number < i386_regtab_size)
>>>>    	{
>>>>    	  exp->X_add_number = i386_regtab[exp->X_add_number]
>>>> -			      .dw2_regnum[flag_code >> 1];
>>>> +			      .dw2_regnum[object_64bit];
>>>>    	  if (exp->X_add_number != Dw2Inval)
>>>>    	    exp->X_op = O_constant;
>>>>    	}
>>>
>>> On one hand, I see that the suggested code changes are making things
>>> semantically clearer, I would like to understand:
>>>
>>> 1. If there is a scenario where flag_code is CODE16_BIT / CODE32_BIT and
>>> object_64bit equal to 1 is supported.  gcc passes --32 when using -m16
>>> or -m32.
>>
>> Well, gcc may never produce such input, but hand-written assembly can.
> 
> Then, should we also use sp[object_64bit] instead of sp[flag_code >> 1] 
> in tc_x86_frame_initial_instructions? Otherwise the assert "gas_assert 
> (exp.X_op == O_constant)" will trigger, e.g. with .code16 and --64.

Indeed. Need to make a v3 then, and probably go hunt for any other
"flag_code >> 1". It's really odd how "well" those uses are scattered
across, with no mention of the need to stay in sync (which wouldn't
have been much of a problem if it had been got right from the
beginning).

Of course I question the invocation of parsing a register name there;
the two numbers that can result are well known. Parsing would perhaps
be warranted if, say, for COFF different numbers would result (and
hence determining them at build time wouldn't be as easy). So I may
end up simplifying this beyond just switching to use of object_64bit.

Jan
diff mbox series

Patch

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -5409,7 +5409,7 @@  ginsn_dw2_regnum (const reg_entry *ireg)
   if (ireg->reg_num == RegIP || ireg->reg_num == RegIZ)
     return GINSN_DW2_REGNUM_RSI_DUMMY;
 
-  dwarf_reg = ireg->dw2_regnum[flag_code >> 1];
+  dwarf_reg = ireg->dw2_regnum[object_64bit];
 
   if (dwarf_reg == Dw2Inval)
     {
@@ -17461,7 +17461,7 @@  tc_x86_parse_to_dw2regnum (expressionS *
       if ((addressT) exp->X_add_number < i386_regtab_size)
 	{
 	  exp->X_add_number = i386_regtab[exp->X_add_number]
-			      .dw2_regnum[flag_code >> 1];
+			      .dw2_regnum[object_64bit];
 	  if (exp->X_add_number != Dw2Inval)
 	    exp->X_op = O_constant;
 	}