[PATCH] frv: fix build failure

[PATCH] frv: fix build failure

[Alexey Brodkin]

Hi Sudip,

On Tue, 2017-11-21@22:10 +0000, Sudip Mukherjee wrote:
> The frv defconfig build is failing with the error:
> lib/mpi/mpih-div.o: In function `mpihelp_divrem':
> mpih-div.c:(.text+0x30c): undefined reference to `abort'
> 
> The function 'abort' was never defined for the frv architecture.
> Create 'abort' as is done in other arch like 'arm' and 'unicore32'.
> 
> Signed-off-by: Sudip Mukherjee <sudipm.mukherjee at gmail.com>
> ---

I'm seeing the same issue building for ARC from today's Linus' tree.
Maybe it worth implementing abort() as a weak function for every arch/platform
that doesn't have it explicitly defined? Otherwise we'll end-up with
useless code duplication.

-Alexey

[PATCH] frv: fix build failure

[Sudip Mukherjee]

Hi Alexey,

On Thu, Nov 23, 2017@05:17:19PM +0000, Alexey Brodkin wrote:
> Hi Sudip,
> 
> On Tue, 2017-11-21@22:10 +0000, Sudip Mukherjee wrote:
> > The frv defconfig build is failing with the error:
> > lib/mpi/mpih-div.o: In function `mpihelp_divrem':
> > mpih-div.c:(.text+0x30c): undefined reference to `abort'
> > 
> > The function 'abort' was never defined for the frv architecture.
> > Create 'abort' as is done in other arch like 'arm' and 'unicore32'.
> > 
> > Signed-off-by: Sudip Mukherjee <sudipm.mukherjee at gmail.com>
> > ---
> 
> I'm seeing the same issue building for ARC from today's Linus' tree.
> Maybe it worth implementing abort() as a weak function for every arch/platform
> that doesn't have it explicitly defined? Otherwise we'll end-up with
> useless code duplication.

Do you mean define it for every arch or define it in a common place so
that all arch can use it?

I did a quick try with the attached patch on two different arch, one of
them (m32r) has the 'abort' defined and the other (frv) was failing due
to lack of abort, and they both built without any warnings or errors.
But I am not sure if 'kernel/exit.c' is the right place for it.
Any suggestion?

--
Regards
Sudip
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch
Type: text/x-diff
Size: 242 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-snps-arc/attachments/20171123/53be03c2/attachment.bin>

[PATCH] frv: fix build failure

[Alexey Brodkin]

Hi Sudip,

On Thu, 2017-11-23@23:01 +0000, Sudip Mukherjee wrote:
> Hi Alexey,
> 
> On Thu, Nov 23, 2017@05:17:19PM +0000, Alexey Brodkin wrote:
> > 
> > Hi Sudip,
> > 
> > On Tue, 2017-11-21@22:10 +0000, Sudip Mukherjee wrote:
> > > 
> > > The frv defconfig build is failing with the error:
> > > lib/mpi/mpih-div.o: In function `mpihelp_divrem':
> > > mpih-div.c:(.text+0x30c): undefined reference to `abort'
> > > 
> > > The function 'abort' was never defined for the frv architecture.
> > > Create 'abort' as is done in other arch like 'arm' and 'unicore32'.
> > > 
> > > Signed-off-by: Sudip Mukherjee <sudipm.mukherjee at gmail.com>
> > > ---
> > 
> > I'm seeing the same issue building for ARC from today's Linus' tree.
> > Maybe it worth implementing abort() as a weak function for every arch/platform
> > that doesn't have it explicitly defined? Otherwise we'll end-up with
> > useless code duplication.
> 
> Do you mean define it for every arch or define it in a common place so
> that all arch can use it?

Essentially I'd prefer to have just 1 instance of this function.
In fact I'd even remove existing implementations in ARM, Unicore32 and M32R
and have a generic implementation as this function obviously has nothing arch-specific.

> I did a quick try with the attached patch on two different arch, one of
> them (m32r) has the 'abort' defined and the other (frv) was failing due
> to lack of abort, and they both built without any warnings or errors.

Well maybe add ?call to panic() there as well as it is done for ARM, unicore32?and M32R?

> But I am not sure if 'kernel/exit.c' is the right place for it.
> Any suggestion?

I guess it should be indeed something in "kernel/" and maybe your existing choice
of "kernel/exit.c" is not bad. Just send a patch to LKML and you'll get much better
guidance on that I guess :)

Anyways thanks for looking at this one.

-Alexey

[PATCH] frv: fix build failure

[Vineet Gupta]

+CC linux-arch, Arnd

On 11/23/2017 09:17 AM, Alexey Brodkin wrote:
> Hi Sudip,
> 
> On Tue, 2017-11-21@22:10 +0000, Sudip Mukherjee wrote:
>> The frv defconfig build is failing with the error:
>> lib/mpi/mpih-div.o: In function `mpihelp_divrem':
>> mpih-div.c:(.text+0x30c): undefined reference to `abort'
>>
>> The function 'abort' was never defined for the frv architecture.
>> Create 'abort' as is done in other arch like 'arm' and 'unicore32'.
>>
>> Signed-off-by: Sudip Mukherjee <sudipm.mukherjee at gmail.com>
>> ---
> 
> I'm seeing the same issue building for ARC from today's Linus' tree.

With which compiler ? Claudiu implemented __builtin_trap recently so ARC builds 
should not get abort() but a trap 5 instruction instead.

> Maybe it worth implementing abort() as a weak function for every arch/platform
> that doesn't have it explicitly defined? Otherwise we'll end-up with
> useless code duplication.

I understand the case/need for adding a weak/common abort() as a quick fix for 
handling such cases, but perhaps we should not and instead fix the rootcause. In 
this specific case, Claudiu mentioned that gcc was generating abort due to 
something like this (flagging a possible divide by zero due to 
-fno-isolate-erroneous-paths-dereference.

a;

fn1() {
   switch (a)
   case 0:
   return 1 / a;
}

Sudeep can you confirm that removing this hacking FRV build to not have this 
toggle fixes the abort issue.

So the offending code needs to be fixed as in the end when that code path is taken 
system is hosed.

-Vineet

[PATCH] frv: fix build failure

[Sudip Mukherjee]

On Mon, Nov 27, 2017@10:25:16AM -0800, Vineet Gupta wrote:
> +CC linux-arch, Arnd
> 
> On 11/23/2017 09:17 AM, Alexey Brodkin wrote:
> >Hi Sudip,
> >
> >On Tue, 2017-11-21@22:10 +0000, Sudip Mukherjee wrote:

<snip>

> I understand the case/need for adding a weak/common abort() as a
> quick fix for handling such cases, but perhaps we should not and
> instead fix the rootcause. In this specific case, Claudiu mentioned
> that gcc was generating abort due to something like this (flagging a
> possible divide by zero due to
> -fno-isolate-erroneous-paths-dereference.
> 
> a;
> 
> fn1() {
>   switch (a)
>   case 0:
>   return 1 / a;
> }
> 
> Sudeep can you confirm that removing this hacking FRV build to not
> have this toggle fixes the abort issue.
> 
> So the offending code needs to be fixed as in the end when that code
> path is taken system is hosed.

The above case that you mentioned is indeed the case here.
mpihelp_divrem() in lib/mpi/mpih-div.c has this same divide by zero case.

So, what should be the actual fix here?

--
Regards
Sudip

[PATCH] frv: fix build failure

[Vineet Gupta]

On 12/03/2017 02:32 PM, Sudip Mukherjee wrote:
> On Mon, Nov 27, 2017@10:25:16AM -0800, Vineet Gupta wrote:
>> +CC linux-arch, Arnd
>>
>> On 11/23/2017 09:17 AM, Alexey Brodkin wrote:
>>> Hi Sudip,
>>>
>>> On Tue, 2017-11-21@22:10 +0000, Sudip Mukherjee wrote:
> 
> <snip>
> 
>> I understand the case/need for adding a weak/common abort() as a
>> quick fix for handling such cases, but perhaps we should not and
>> instead fix the rootcause. In this specific case, Claudiu mentioned
>> that gcc was generating abort due to something like this (flagging a
>> possible divide by zero due to
>> -fno-isolate-erroneous-paths-dereference.
>>
>> a;
>>
>> fn1() {
>>    switch (a)
>>    case 0:
>>    return 1 / a;
>> }
>>
>> Sudeep can you confirm that removing this hacking FRV build to not
>> have this toggle fixes the abort issue.
>>
>> So the offending code needs to be fixed as in the end when that code
>> path is taken system is hosed.
> 
> The above case that you mentioned is indeed the case here.
> mpihelp_divrem() in lib/mpi/mpih-div.c has this same divide by zero case.
> 
> So, what should be the actual fix here?

To remove the divide by zero.

Or atleast have a __weak abort() defined in common code so all arches benefit from 
it !

-Vineet