Re: [PATCH] [i386] PR 81850 Don't ignore -mabi=sysv on Cygwin/MinGW

[Daniel Santos]

On 08/23/2017 01:12 AM, Uros Bizjak wrote:
> On Wed, Aug 23, 2017 at 7:23 AM, Daniel Santos  
> wrote:
>> On 08/22/2017 03:00 PM, Uros Bizjak wrote:
>>> On Tue, Aug 22, 2017 at 9:47 PM, Daniel Santos  
>>> wrote:
> Please add UNKNOWN_ABI to the enum and initialize -mabi in i386.opt to
> UNKNOWN_ABI.
 It would seem to me that UNSPECIFIED_ABI would be a better value name.

 Also, I don't really understand what opts_set and opts are, except that I 
 had
 guessed opts_set is what the user asked for (or didn't ask for) and opts is
 what we're going to actually use.  Am I close?
>>> Yes. opts_set is a flag that user specified an option at the command line.
>>>
>>> However, I fail to see what is the problem. If nothing was specified,
>>> then opts->x_ix86_abi is set to DEFAULT_ABI.
>> That is not what is happening.  If -mabi=sysv is specified, then the
>> test (!opts_set->x_ix86_abi) is true since the value of SYSV_ABI is
>> zero.  When that is evaluated as true, then the abi is set to
>> DEFAULT_ABI, which on Windows is MS_ABI, thus ignoring the command line
>> option.
> Let's use the following patch:
>
> --cut here--
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index 3c82ae64f4f2..f8590f663285 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -5682,7 +5682,7 @@ ix86_option_override_internal (bool main_args_p,
>  ? PMODE_DI : PMODE_SI;
>
>if (!opts_set->x_ix86_abi)
> -opts->x_ix86_abi = DEFAULT_ABI;
> +printf ("Using default ABI\n"), opts->x_ix86_abi = DEFAULT_ABI;
>
>/* For targets using ms ABI enable ms-extensions, if not
>   explicit turned off.  For non-ms ABI we turn off this
> --cut here--
>
> $ ./cc1 -O2 -quiet hello.c
> Using default ABI
> $ ./cc1 -O2 -mabi=sysv -quiet hello.c
> $
> $ ./cc1 -O2 -mabi=sysv -quiet hello.c
> $
>
> Again, opts_set is set to true when the option is specified on the
> command line, it has nothing to do with the value of the option.

Interesting, I get the same result and in fact I can't reproduce the bug
anymore.  Either I made a mistake somewhere (likely) or something else
fixed the problem (less likely).  I'll try again from where the trunk
was when I filed the bug and close it either invalid or fixed depending
upon which it is.

Thanks!
Daniel

>> I'm guessing that if we don't specify an Init() option then it will
>> default to zero?  We just need a valid way to differentiate when
>> -mabi=sysv has been passed from when nothing has been passed.
> Yes, it defaults to zero, but since we live in c++ world nowadays, we
> can't initialize enum with integer zero...
>
> Uros.

Re: [PATCH] [i386] PR 81850 Don't ignore -mabi=sysv on Cygwin/MinGW

[Uros Bizjak]

On Wed, Aug 23, 2017 at 7:23 AM, Daniel Santos  wrote:
> On 08/22/2017 03:00 PM, Uros Bizjak wrote:
>> On Tue, Aug 22, 2017 at 9:47 PM, Daniel Santos  
>> wrote:
 Please add UNKNOWN_ABI to the enum and initialize -mabi in i386.opt to
 UNKNOWN_ABI.
>>> It would seem to me that UNSPECIFIED_ABI would be a better value name.
>>>
>>> Also, I don't really understand what opts_set and opts are, except that I 
>>> had
>>> guessed opts_set is what the user asked for (or didn't ask for) and opts is
>>> what we're going to actually use.  Am I close?
>> Yes. opts_set is a flag that user specified an option at the command line.
>>
>> However, I fail to see what is the problem. If nothing was specified,
>> then opts->x_ix86_abi is set to DEFAULT_ABI.
>
> That is not what is happening.  If -mabi=sysv is specified, then the
> test (!opts_set->x_ix86_abi) is true since the value of SYSV_ABI is
> zero.  When that is evaluated as true, then the abi is set to
> DEFAULT_ABI, which on Windows is MS_ABI, thus ignoring the command line
> option.

Let's use the following patch:

--cut here--
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 3c82ae64f4f2..f8590f663285 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -5682,7 +5682,7 @@ ix86_option_override_internal (bool main_args_p,
 ? PMODE_DI : PMODE_SI;

   if (!opts_set->x_ix86_abi)
-opts->x_ix86_abi = DEFAULT_ABI;
+printf ("Using default ABI\n"), opts->x_ix86_abi = DEFAULT_ABI;

   /* For targets using ms ABI enable ms-extensions, if not
  explicit turned off.  For non-ms ABI we turn off this
--cut here--

$ ./cc1 -O2 -quiet hello.c
Using default ABI
$ ./cc1 -O2 -mabi=sysv -quiet hello.c
$
$ ./cc1 -O2 -mabi=sysv -quiet hello.c
$

Again, opts_set is set to true when the option is specified on the
command line, it has nothing to do with the value of the option.

> I'm guessing that if we don't specify an Init() option then it will
> default to zero?  We just need a valid way to differentiate when
> -mabi=sysv has been passed from when nothing has been passed.

Yes, it defaults to zero, but since we live in c++ world nowadays, we
can't initialize enum with integer zero...

Uros.

Re: [PATCH] [i386] PR 81850 Don't ignore -mabi=sysv on Cygwin/MinGW

[Daniel Santos]

On 08/22/2017 03:00 PM, Uros Bizjak wrote:
> On Tue, Aug 22, 2017 at 9:47 PM, Daniel Santos  
> wrote:
>>> Please add UNKNOWN_ABI to the enum and initialize -mabi in i386.opt to
>>> UNKNOWN_ABI.
>> It would seem to me that UNSPECIFIED_ABI would be a better value name.
>>
>> Also, I don't really understand what opts_set and opts are, except that I had
>> guessed opts_set is what the user asked for (or didn't ask for) and opts is
>> what we're going to actually use.  Am I close?
> Yes. opts_set is a flag that user specified an option at the command line.
>
> However, I fail to see what is the problem. If nothing was specified,
> then opts->x_ix86_abi is set to DEFAULT_ABI.

That is not what is happening.  If -mabi=sysv is specified, then the
test (!opts_set->x_ix86_abi) is true since the value of SYSV_ABI is
zero.  When that is evaluated as true, then the abi is set to
DEFAULT_ABI, which on Windows is MS_ABI, thus ignoring the command line
option.

> Probably we don't need
> Init(SYSV_ABI) in mabi= declaration at all.

I'm guessing that if we don't specify an Init() option then it will
default to zero?  We just need a valid way to differentiate when
-mabi=sysv has been passed from when nothing has been passed.

Daniel

>
> Uros.
>
>> I'm re-running tests, so if they pass is this OK?
>>
>> Thanks,
>> Daniel
>> ---
>>  gcc/config/i386/i386-opts.h | 5 +++--
>>  gcc/config/i386/i386.c  | 3 +--
>>  gcc/config/i386/i386.opt| 2 +-
>>  3 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/gcc/config/i386/i386-opts.h b/gcc/config/i386/i386-opts.h
>> index 542cd0f3d67..a1d1552a3c6 100644
>> --- a/gcc/config/i386/i386-opts.h
>> +++ b/gcc/config/i386/i386-opts.h
>> @@ -44,8 +44,9 @@ last_alg
>>  /* Available call abi.  */
>>  enum calling_abi
>>  {
>> -  SYSV_ABI = 0,
>> -  MS_ABI = 1
>> +  UNSPECIFIED_ABI = 0,
>> +  SYSV_ABI = 1,
>> +  MS_ABI = 2
>>  };
>>
>>  enum fpmath_unit
>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>> index 650bcbc65ae..c08ad55fcd9 100644
>> --- a/gcc/config/i386/i386.c
>> +++ b/gcc/config/i386/i386.c
>> @@ -5681,12 +5681,11 @@ ix86_option_override_internal (bool main_args_p,
>>  opts->x_ix86_pmode = TARGET_LP64_P (opts->x_ix86_isa_flags)
>>  ? PMODE_DI : PMODE_SI;
>>
>> -  if (!opts_set->x_ix86_abi)
>> +  if (opts_set->x_ix86_abi == UNSPECIFIED_ABI)
>>  opts->x_ix86_abi = DEFAULT_ABI;
>>
>>if (opts->x_ix86_abi == MS_ABI && TARGET_X32_P (opts->x_ix86_isa_flags))
>>  error ("-mabi=ms not supported with X32 ABI");
>> -  gcc_assert (opts->x_ix86_abi == SYSV_ABI || opts->x_ix86_abi == MS_ABI);
>>
>>/* For targets using ms ABI enable ms-extensions, if not
>>   explicit turned off.  For non-ms ABI we turn off this
>> diff --git a/gcc/config/i386/i386.opt b/gcc/config/i386/i386.opt
>> index cd564315f04..f7b9f9707f7 100644
>> --- a/gcc/config/i386/i386.opt
>> +++ b/gcc/config/i386/i386.opt
>> @@ -525,7 +525,7 @@ Target Report Mask(IAMCU)
>>  Generate code that conforms to Intel MCU psABI.
>>
>>  mabi=
>> -Target RejectNegative Joined Var(ix86_abi) Enum(calling_abi) Init(SYSV_ABI)
>> +Target RejectNegative Joined Var(ix86_abi) Enum(calling_abi) 
>> Init(UNSPECIFIED_ABI)
>>  Generate code that conforms to the given ABI.
>>
>>  Enum
>> --
>> 2.13.3
>>

Re: [PATCH] [i386] PR 81850 Don't ignore -mabi=sysv on Cygwin/MinGW

[JonY]

On 08/22/2017 06:32 AM, Uros Bizjak wrote:
> On Tue, Aug 22, 2017 at 4:10 AM, Daniel Santos  
> wrote:
> 
>> This is a problem that occured because of this code in
>> ix86_option_override_internal:
>>
>>   if (!opts_set->x_ix86_abi)
>> opts->x_ix86_abi = DEFAULT_ABI;
>>
>> I tested this along with my other patches.  OK for trunk?
>>
>> * config/i386/i386-opts.h (enum calling_abi): Modify so that no legal
>> values are equivalent to zero.
> 
> Please add UNKNOWN_ABI to the enum and initialize -mabi in i386.opt to
> UNKNOWN_ABI.
> 
> Then change the above condition to
> 
> if (opts_set->x_ix86_abi == UNKNOWN_ABI)
> 
> We can't just init -mabi to DEFAULT_ABI, sinde this is selected at
> runtime. Maybe a comment should be added for UNKNOWN_ABI, that it is
> overriden in ix86_option_override_internal.
> 
> Uros.

This sounds sensible if there was ever a time some new ABI is added for x86.



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] [i386] PR 81850 Don't ignore -mabi=sysv on Cygwin/MinGW

[Uros Bizjak]

On Tue, Aug 22, 2017 at 9:47 PM, Daniel Santos  wrote:
>> Please add UNKNOWN_ABI to the enum and initialize -mabi in i386.opt to
>> UNKNOWN_ABI.
>
> It would seem to me that UNSPECIFIED_ABI would be a better value name.
>
> Also, I don't really understand what opts_set and opts are, except that I had
> guessed opts_set is what the user asked for (or didn't ask for) and opts is
> what we're going to actually use.  Am I close?

Yes. opts_set is a flag that user specified an option at the command line.

However, I fail to see what is the problem. If nothing was specified,
then opts->x_ix86_abi is set to DEFAULT_ABI. Probably we don't need
Init(SYSV_ABI) in mabi= declaration at all.

Uros.

> I'm re-running tests, so if they pass is this OK?
>
> Thanks,
> Daniel
> ---
>  gcc/config/i386/i386-opts.h | 5 +++--
>  gcc/config/i386/i386.c  | 3 +--
>  gcc/config/i386/i386.opt| 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/gcc/config/i386/i386-opts.h b/gcc/config/i386/i386-opts.h
> index 542cd0f3d67..a1d1552a3c6 100644
> --- a/gcc/config/i386/i386-opts.h
> +++ b/gcc/config/i386/i386-opts.h
> @@ -44,8 +44,9 @@ last_alg
>  /* Available call abi.  */
>  enum calling_abi
>  {
> -  SYSV_ABI = 0,
> -  MS_ABI = 1
> +  UNSPECIFIED_ABI = 0,
> +  SYSV_ABI = 1,
> +  MS_ABI = 2
>  };
>
>  enum fpmath_unit
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index 650bcbc65ae..c08ad55fcd9 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -5681,12 +5681,11 @@ ix86_option_override_internal (bool main_args_p,
>  opts->x_ix86_pmode = TARGET_LP64_P (opts->x_ix86_isa_flags)
>  ? PMODE_DI : PMODE_SI;
>
> -  if (!opts_set->x_ix86_abi)
> +  if (opts_set->x_ix86_abi == UNSPECIFIED_ABI)
>  opts->x_ix86_abi = DEFAULT_ABI;
>
>if (opts->x_ix86_abi == MS_ABI && TARGET_X32_P (opts->x_ix86_isa_flags))
>  error ("-mabi=ms not supported with X32 ABI");
> -  gcc_assert (opts->x_ix86_abi == SYSV_ABI || opts->x_ix86_abi == MS_ABI);
>
>/* For targets using ms ABI enable ms-extensions, if not
>   explicit turned off.  For non-ms ABI we turn off this
> diff --git a/gcc/config/i386/i386.opt b/gcc/config/i386/i386.opt
> index cd564315f04..f7b9f9707f7 100644
> --- a/gcc/config/i386/i386.opt
> +++ b/gcc/config/i386/i386.opt
> @@ -525,7 +525,7 @@ Target Report Mask(IAMCU)
>  Generate code that conforms to Intel MCU psABI.
>
>  mabi=
> -Target RejectNegative Joined Var(ix86_abi) Enum(calling_abi) Init(SYSV_ABI)
> +Target RejectNegative Joined Var(ix86_abi) Enum(calling_abi) 
> Init(UNSPECIFIED_ABI)
>  Generate code that conforms to the given ABI.
>
>  Enum
> --
> 2.13.3
>

[PATCH] [i386] PR 81850 Don't ignore -mabi=sysv on Cygwin/MinGW

[Daniel Santos]

> Please add UNKNOWN_ABI to the enum and initialize -mabi in i386.opt to
> UNKNOWN_ABI.

It would seem to me that UNSPECIFIED_ABI would be a better value name.

Also, I don't really understand what opts_set and opts are, except that I had
guessed opts_set is what the user asked for (or didn't ask for) and opts is
what we're going to actually use.  Am I close?

I'm re-running tests, so if they pass is this OK?

Thanks,
Daniel
---
 gcc/config/i386/i386-opts.h | 5 +++--
 gcc/config/i386/i386.c  | 3 +--
 gcc/config/i386/i386.opt| 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/gcc/config/i386/i386-opts.h b/gcc/config/i386/i386-opts.h
index 542cd0f3d67..a1d1552a3c6 100644
--- a/gcc/config/i386/i386-opts.h
+++ b/gcc/config/i386/i386-opts.h
@@ -44,8 +44,9 @@ last_alg
 /* Available call abi.  */
 enum calling_abi
 {
-  SYSV_ABI = 0,
-  MS_ABI = 1
+  UNSPECIFIED_ABI = 0,
+  SYSV_ABI = 1,
+  MS_ABI = 2
 };
 
 enum fpmath_unit
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 650bcbc65ae..c08ad55fcd9 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -5681,12 +5681,11 @@ ix86_option_override_internal (bool main_args_p,
 opts->x_ix86_pmode = TARGET_LP64_P (opts->x_ix86_isa_flags)
 ? PMODE_DI : PMODE_SI;
 
-  if (!opts_set->x_ix86_abi)
+  if (opts_set->x_ix86_abi == UNSPECIFIED_ABI)
 opts->x_ix86_abi = DEFAULT_ABI;
 
   if (opts->x_ix86_abi == MS_ABI && TARGET_X32_P (opts->x_ix86_isa_flags))
 error ("-mabi=ms not supported with X32 ABI");
-  gcc_assert (opts->x_ix86_abi == SYSV_ABI || opts->x_ix86_abi == MS_ABI);
 
   /* For targets using ms ABI enable ms-extensions, if not
  explicit turned off.  For non-ms ABI we turn off this
diff --git a/gcc/config/i386/i386.opt b/gcc/config/i386/i386.opt
index cd564315f04..f7b9f9707f7 100644
--- a/gcc/config/i386/i386.opt
+++ b/gcc/config/i386/i386.opt
@@ -525,7 +525,7 @@ Target Report Mask(IAMCU)
 Generate code that conforms to Intel MCU psABI.
 
 mabi=
-Target RejectNegative Joined Var(ix86_abi) Enum(calling_abi) Init(SYSV_ABI)
+Target RejectNegative Joined Var(ix86_abi) Enum(calling_abi) 
Init(UNSPECIFIED_ABI)
 Generate code that conforms to the given ABI.
 
 Enum
-- 
2.13.3

Re: [PATCH] [i386] PR 81850 Don't ignore -mabi=sysv on Cygwin/MinGW

[Daniel Santos]

On 08/22/2017 01:26 AM, Andreas Schwab wrote:
> On Aug 21 2017, Daniel Santos  wrote:
>
>> This is a problem that occured because of this code in
>> ix86_option_override_internal:
>>
>>   if (!opts_set->x_ix86_abi)
>> opts->x_ix86_abi = DEFAULT_ABI;
> Why is that a problem?  Note opts_set vs opts.

Just because the test !opts_set->x_ix86_abi will be true rather we
supplied no -mabi parameter or we supplied -mabi=sysv.

Daniel

> Andreas.

Re: [PATCH] [i386] PR 81850 Don't ignore -mabi=sysv on Cygwin/MinGW

[Uros Bizjak]

On Tue, Aug 22, 2017 at 8:26 AM, Andreas Schwab  wrote:
> On Aug 21 2017, Daniel Santos  wrote:
>
>> This is a problem that occured because of this code in
>> ix86_option_override_internal:
>>
>>   if (!opts_set->x_ix86_abi)
>> opts->x_ix86_abi = DEFAULT_ABI;
>
> Why is that a problem?  Note opts_set vs opts.

Uh, indeed. Need to clean my glasses. The above is a flag that
something is specified at command line.

Uros.

> Andreas.
>
> --
> Andreas Schwab, SUSE Labs, sch...@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."

Re: [PATCH] [i386] PR 81850 Don't ignore -mabi=sysv on Cygwin/MinGW

[Uros Bizjak]

On Tue, Aug 22, 2017 at 4:10 AM, Daniel Santos  wrote:

> This is a problem that occured because of this code in
> ix86_option_override_internal:
>
>   if (!opts_set->x_ix86_abi)
> opts->x_ix86_abi = DEFAULT_ABI;
>
> I tested this along with my other patches.  OK for trunk?
>
> * config/i386/i386-opts.h (enum calling_abi): Modify so that no legal
> values are equivalent to zero.

Please add UNKNOWN_ABI to the enum and initialize -mabi in i386.opt to
UNKNOWN_ABI.

Then change the above condition to

if (opts_set->x_ix86_abi == UNKNOWN_ABI)

We can't just init -mabi to DEFAULT_ABI, sinde this is selected at
runtime. Maybe a comment should be added for UNKNOWN_ABI, that it is
overriden in ix86_option_override_internal.

Uros.

> Thanks,
> Daniel
>
> Signed-off-by: Daniel Santos 
> ---
>  gcc/config/i386/i386-opts.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/config/i386/i386-opts.h b/gcc/config/i386/i386-opts.h
> index 542cd0f3d67..8c2b5380e49 100644
> --- a/gcc/config/i386/i386-opts.h
> +++ b/gcc/config/i386/i386-opts.h
> @@ -44,8 +44,8 @@ last_alg
>  /* Available call abi.  */
>  enum calling_abi
>  {
> -  SYSV_ABI = 0,
> -  MS_ABI = 1
> +  SYSV_ABI = 1,
> +  MS_ABI = 2
>  };
>
>  enum fpmath_unit
> --
> 2.13.3
>

Re: [PATCH] [i386] PR 81850 Don't ignore -mabi=sysv on Cygwin/MinGW

[Andreas Schwab]

On Aug 21 2017, Daniel Santos  wrote:

> This is a problem that occured because of this code in
> ix86_option_override_internal:
>
>   if (!opts_set->x_ix86_abi)
> opts->x_ix86_abi = DEFAULT_ABI;

Why is that a problem?  Note opts_set vs opts.

Andreas.

-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

[PATCH] [i386] PR 81850 Don't ignore -mabi=sysv on Cygwin/MinGW

[Daniel Santos]

This is a problem that occured because of this code in
ix86_option_override_internal:

  if (!opts_set->x_ix86_abi)
opts->x_ix86_abi = DEFAULT_ABI;

I tested this along with my other patches.  OK for trunk?

* config/i386/i386-opts.h (enum calling_abi): Modify so that no legal
values are equivalent to zero.

Thanks,
Daniel

Signed-off-by: Daniel Santos 
---
 gcc/config/i386/i386-opts.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gcc/config/i386/i386-opts.h b/gcc/config/i386/i386-opts.h
index 542cd0f3d67..8c2b5380e49 100644
--- a/gcc/config/i386/i386-opts.h
+++ b/gcc/config/i386/i386-opts.h
@@ -44,8 +44,8 @@ last_alg
 /* Available call abi.  */
 enum calling_abi
 {
-  SYSV_ABI = 0,
-  MS_ABI = 1
+  SYSV_ABI = 1,
+  MS_ABI = 2
 };
 
 enum fpmath_unit
-- 
2.13.3