PR83775

PR83775

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 114 bytes --]

Hi,
The attached patch tries to fix PR83775.
Validation in progress.
OK to commit if passes ?

Thanks,
Prathamesh

[-- Attachment #2: pr83775-1.txt --]
[-- Type: text/plain, Size: 838 bytes --]

2018-01-11  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>

	PR target/83775
	* config/arm/arm.c (arm_declare_function_name): Set arch_to_print if
	targ_options->x_arm_arch_string is non NULL.

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 196aa6de1ac..868251a154c 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -30954,7 +30954,10 @@ arm_declare_function_name (FILE *stream, const char *name, tree decl)
 
   /* Only update the assembler .arch string if it is distinct from the last
      such string we printed.  */
-  std::string arch_to_print = targ_options->x_arm_arch_string;
+  std::string arch_to_print;
+  if (targ_options->x_arm_arch_string)
+    arch_to_print = targ_options->x_arm_arch_string;
+
   if (arch_to_print != arm_last_printed_arch_string)
     {
       std::string arch_name

Re: PR83775

[Martin Sebor]

On 01/10/2018 12:01 PM, Prathamesh Kulkarni wrote:
> Hi,
> The attached patch tries to fix PR83775.
> Validation in progress.
> OK to commit if passes ?

FWIW, the patch makes sense to me as it simplifies things for
me when debugging using a cross-compiler.  I reported the same
ICE in bug 83775 and it was closed as WontFix but perhaps with
a patch the decision will be reconsidered.  It's not very user
friendly to crash on the wrong/missing options.  If the patch
isn't accepted then perhaps one where the compiler issues
an error would be.

Martin

Re: PR83775

[Jeff Law]

On 01/10/2018 12:21 PM, Martin Sebor wrote:
> On 01/10/2018 12:01 PM, Prathamesh Kulkarni wrote:
>> Hi,
>> The attached patch tries to fix PR83775.
>> Validation in progress.
>> OK to commit if passes ?
> 
> FWIW, the patch makes sense to me as it simplifies things for
> me when debugging using a cross-compiler.  I reported the same
> ICE in bug 83775 and it was closed as WontFix but perhaps with
> a patch the decision will be reconsidered.  It's not very user
> friendly to crash on the wrong/missing options.  If the patch
> isn't accepted then perhaps one where the compiler issues
> an error would be.
I've found the current behavior extremely user unfriendly as well.

jeff

Re: PR83775

[Kyrill Tkachov]

Hi Prathamesh,

On 10/01/18 19:01, Prathamesh Kulkarni wrote:
> Hi,
> The attached patch tries to fix PR83775.
> Validation in progress.
> OK to commit if passes ?
>

I've read the relevant PRs and other's inputs on this
(thanks for the perspectives!)
I agree with Richard's design that the driver does all
the smarts of processing the options to create the
right -march string. There's a lot of options,
extensions and variants that we need to keep
track of and having it all in one place in the driver
seems sensible as we need that information there to
drive many other things like multilib selection and
option string rewriting for the other parts of the toolchain.

However, I do agree that being able to debug cc1 directly
is indispensable when working with cross-compilers.
I myself only have the capability of building just a cc1 for
most non-arm (and aarch64) targets when I need to investigate
issues on them. So I'm keen on fixing this segfault.

At this point the end-user-facing experience really requires
them to use the driver to ensure that the assembler, linker
options are correct and the right libraries are selected.
The .arch string we output is for the benefit of the
assembler and if we the user wants the interface to the
assembler to work correctly they should be using the driver
anyway (A cc1 -> gas -> ld pipeline is not a supported pipeline AFAIK).
So setting arch_to_print to the empty string here in order
to not crash looks like a reasonable fix.
Is there some way we can write an assert for the
targ_options->x_arm_arch_string == NULL case to check that we
are indeed invoked without a driver? If so, can you please add one?
Otherwise add a comment saying that cc1 has been invoked directly.

This patch is ok for trunk with either of those changes.

Thanks,
Kyrill

> Thanks,
> Prathamesh