RISC-V: Disable attribute generation by default

Message ID 20230224022439.18998-1-palmer@rivosinc.com
State Rejected
Delegated to: Kito Cheng
Headers
Series RISC-V: Disable attribute generation by default |

Commit Message

Palmer Dabbelt Feb. 24, 2023, 2:24 a.m. UTC
  We generate a handful of attributes by default, but they don't really
encode any useful information.  We've broadly stopped ascribing any
meaning to them in binutils; but they trip up LLVM, older toolchains,
and users.  So let's just turn them off by default.  The old binaries
will still be floating around, but at least this way we'll stop tripping
over new incompatibilities.

If we get to a point where there's some attributes that are defined that
we can use then we can sort out how to turn those on without turning on
the old ones, but unless I'm missing something the current set of
attributes are too broken to be useful for anything.

gcc/ChangeLog:

	* config.gcc (--with-riscv-attribute): Default to off.
---
I know it's pretty late, but I'd like to target this for GCC-13.  The
Zmmul stuff has resulted in another round of build breakages that we're
going to have to chase down, and while we could update everything to
turn off the attributes it seems easier to just set the default.
---
 gcc/config.gcc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Comments

Kito Cheng Feb. 24, 2023, 1:09 p.m. UTC | #1
It did help people to identify what extension used in the binary, so I
would prefer keep that enable by default.

and lld is begin fix those merge issue, so the situation should be improved
soon.


Palmer Dabbelt <palmer@rivosinc.com> 於 2023年2月24日 週五 10:29 寫道:

> We generate a handful of attributes by default, but they don't really
> encode any useful information.  We've broadly stopped ascribing any
> meaning to them in binutils; but they trip up LLVM, older toolchains,
> and users.  So let's just turn them off by default.  The old binaries
> will still be floating around, but at least this way we'll stop tripping
> over new incompatibilities.
>
> If we get to a point where there's some attributes that are defined that
> we can use then we can sort out how to turn those on without turning on
> the old ones, but unless I'm missing something the current set of
> attributes are too broken to be useful for anything.
>
> gcc/ChangeLog:
>
>         * config.gcc (--with-riscv-attribute): Default to off.
> ---
> I know it's pretty late, but I'd like to target this for GCC-13.  The
> Zmmul stuff has resulted in another round of build breakages that we're
> going to have to chase down, and while we could update everything to
> turn off the attributes it seems easier to just set the default.
> ---
>  gcc/config.gcc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/config.gcc b/gcc/config.gcc
> index c070e6ecd2e..52639cf26d6 100644
> --- a/gcc/config.gcc
> +++ b/gcc/config.gcc
> @@ -4596,7 +4596,7 @@ case "${target}" in
>                         tm_defines="${tm_defines} TARGET_RISCV_ATTRIBUTE=0"
>                         ;;
>                 ""|default)
> -                       tm_defines="${tm_defines} TARGET_RISCV_ATTRIBUTE=1"
> +                       tm_defines="${tm_defines} TARGET_RISCV_ATTRIBUTE=0"
>                         ;;
>                 *)
>                         echo
> "--with-riscv-attribute=${with_riscv_attribute} is not supported.  The
> argument must begin with yes, no or default." 1>&2
> --
> 2.39.1
>
>
  
Palmer Dabbelt Feb. 25, 2023, 12:42 a.m. UTC | #2
On Fri, 24 Feb 2023 05:09:30 PST (-0800), gcc-patches@gcc.gnu.org wrote:
> It did help people to identify what extension used in the binary, so I
> would prefer keep that enable by default.

IMO it actually hurts more than helps, as it's not really encoding what 
extensions are in the binary (or necessary to run the binary) but 
instead just encodes what was in -march (with some noise added due to 
the merging bugs and ISA string changes).  Having the attributes just 
ends up tricking users into thinking the information is accurate when 
it's not.

> and lld is begin fix those merge issue, so the situation should be improved
> soon.

If toolchains are just going to ignore then attributes then it's a 
pretty good sign they're not useful.

> Palmer Dabbelt <palmer@rivosinc.com> 於 2023年2月24日 週五 10:29 寫道:
>
>> We generate a handful of attributes by default, but they don't really
>> encode any useful information.  We've broadly stopped ascribing any
>> meaning to them in binutils; but they trip up LLVM, older toolchains,
>> and users.  So let's just turn them off by default.  The old binaries
>> will still be floating around, but at least this way we'll stop tripping
>> over new incompatibilities.
>>
>> If we get to a point where there's some attributes that are defined that
>> we can use then we can sort out how to turn those on without turning on
>> the old ones, but unless I'm missing something the current set of
>> attributes are too broken to be useful for anything.
>>
>> gcc/ChangeLog:
>>
>>         * config.gcc (--with-riscv-attribute): Default to off.
>> ---
>> I know it's pretty late, but I'd like to target this for GCC-13.  The
>> Zmmul stuff has resulted in another round of build breakages that we're
>> going to have to chase down, and while we could update everything to
>> turn off the attributes it seems easier to just set the default.
>> ---
>>  gcc/config.gcc | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gcc/config.gcc b/gcc/config.gcc
>> index c070e6ecd2e..52639cf26d6 100644
>> --- a/gcc/config.gcc
>> +++ b/gcc/config.gcc
>> @@ -4596,7 +4596,7 @@ case "${target}" in
>>                         tm_defines="${tm_defines} TARGET_RISCV_ATTRIBUTE=0"
>>                         ;;
>>                 ""|default)
>> -                       tm_defines="${tm_defines} TARGET_RISCV_ATTRIBUTE=1"
>> +                       tm_defines="${tm_defines} TARGET_RISCV_ATTRIBUTE=0"
>>                         ;;
>>                 *)
>>                         echo
>> "--with-riscv-attribute=${with_riscv_attribute} is not supported.  The
>> argument must begin with yes, no or default." 1>&2
>> --
>> 2.39.1
>>
>>
  

Patch

diff --git a/gcc/config.gcc b/gcc/config.gcc
index c070e6ecd2e..52639cf26d6 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -4596,7 +4596,7 @@  case "${target}" in
 			tm_defines="${tm_defines} TARGET_RISCV_ATTRIBUTE=0"
 			;;
 		""|default)
-			tm_defines="${tm_defines} TARGET_RISCV_ATTRIBUTE=1"
+			tm_defines="${tm_defines} TARGET_RISCV_ATTRIBUTE=0"
 			;;
 		*)
 			echo "--with-riscv-attribute=${with_riscv_attribute} is not supported.  The argument must begin with yes, no or default." 1>&2