diff mbox

build-many-glibcs: Remove no_isolate from SH config

Message ID 1489403315-6856-1-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show

Commit Message

Adhemerval Zanella March 13, 2017, 11:08 a.m. UTC 
Now with d40dbe7 SH build does not require more the no_isolate gcc
options to correct build glibc (since SH build now does not generate
a trap anymore).  This patch removes the unrequired options from
SH config.

Checked with a build for sh3-linux-gnu, sh3eb-linux-gnu, sh4-linux-gnu,
and sh4eb-linux-gnu.

	* scripts/build-many-glibcs.py (Context.add_all_configs): Remove
	no_isolate usage for SH.
---
 ChangeLog                    |  5 +++++
 scripts/build-many-glibcs.py | 23 ++++++++---------------
 2 files changed, 13 insertions(+), 15 deletions(-)

Comments

Joseph Myers March 13, 2017, 4:40 p.m. UTC | #1 
On Mon, 13 Mar 2017, Adhemerval Zanella wrote:

> Now with d40dbe7 SH build does not require more the no_isolate gcc
> options to correct build glibc (since SH build now does not generate
> a trap anymore).  This patch removes the unrequired options from
> SH config.

no_isolate is *only* used for SH, so should be removed completely if 
removed from use for SH.
Adhemerval Zanella March 13, 2017, 5:48 p.m. UTC | #2 
On 13/03/2017 13:40, Joseph Myers wrote:
> On Mon, 13 Mar 2017, Adhemerval Zanella wrote:
> 
>> Now with d40dbe7 SH build does not require more the no_isolate gcc
>> options to correct build glibc (since SH build now does not generate
>> a trap anymore).  This patch removes the unrequired options from
>> SH config.
> 
> no_isolate is *only* used for SH, so should be removed completely if 
> removed from use for SH.
> 

Right, I thought about letting the options for possible other future
architectures that might face the same issue.  I will remove it.
Joseph Myers March 13, 2017, 11:17 p.m. UTC | #3 
On Mon, 13 Mar 2017, Adhemerval Zanella wrote:

> On 13/03/2017 13:40, Joseph Myers wrote:
> > On Mon, 13 Mar 2017, Adhemerval Zanella wrote:
> > 
> >> Now with d40dbe7 SH build does not require more the no_isolate gcc
> >> options to correct build glibc (since SH build now does not generate
> >> a trap anymore).  This patch removes the unrequired options from
> >> SH config.
> > 
> > no_isolate is *only* used for SH, so should be removed completely if 
> > removed from use for SH.
> > 
> 
> Right, I thought about letting the options for possible other future
> architectures that might face the same issue.  I will remove it.

It seems this broke the build for SH with GCC mainline (same 
multiple-definitions errors that are symptoms of needing these options); 
please revert.

https://sourceware.org/ml/libc-testresults/2017-q1/msg00248.html
Adhemerval Zanella March 13, 2017, 11:44 p.m. UTC | #4 
On 13/03/2017 20:17, Joseph Myers wrote:
> On Mon, 13 Mar 2017, Adhemerval Zanella wrote:
> 
>> On 13/03/2017 13:40, Joseph Myers wrote:
>>> On Mon, 13 Mar 2017, Adhemerval Zanella wrote:
>>>
>>>> Now with d40dbe7 SH build does not require more the no_isolate gcc
>>>> options to correct build glibc (since SH build now does not generate
>>>> a trap anymore).  This patch removes the unrequired options from
>>>> SH config.
>>>
>>> no_isolate is *only* used for SH, so should be removed completely if 
>>> removed from use for SH.
>>>
>>
>> Right, I thought about letting the options for possible other future
>> architectures that might face the same issue.  I will remove it.
> 
> It seems this broke the build for SH with GCC mainline (same 
> multiple-definitions errors that are symptoms of needing these options); 
> please revert.
> 
> https://sourceware.org/ml/libc-testresults/2017-q1/msg00248.html
> 

Right, I will revert and check what is not failing with default
version.
Adhemerval Zanella March 14, 2017, 11:20 a.m. UTC | #5 
On 13/03/2017 20:44, Adhemerval Zanella wrote:
> 
> 
> On 13/03/2017 20:17, Joseph Myers wrote:
>> On Mon, 13 Mar 2017, Adhemerval Zanella wrote:
>>
>>> On 13/03/2017 13:40, Joseph Myers wrote:
>>>> On Mon, 13 Mar 2017, Adhemerval Zanella wrote:
>>>>
>>>>> Now with d40dbe7 SH build does not require more the no_isolate gcc
>>>>> options to correct build glibc (since SH build now does not generate
>>>>> a trap anymore).  This patch removes the unrequired options from
>>>>> SH config.
>>>>
>>>> no_isolate is *only* used for SH, so should be removed completely if 
>>>> removed from use for SH.
>>>>
>>>
>>> Right, I thought about letting the options for possible other future
>>> architectures that might face the same issue.  I will remove it.
>>
>> It seems this broke the build for SH with GCC mainline (same 
>> multiple-definitions errors that are symptoms of needing these options); 
>> please revert.
>>
>> https://sourceware.org/ml/libc-testresults/2017-q1/msg00248.html
>>
> 
> Right, I will revert and check what is not failing with default
> version.
> 

Joseph, I tracked down the issue and it is due the snippet:

sysdeps/wordsize-32/divdi3.c:

133       else
134         {
135           /* qq = NN / 0d */
136 
137           if (d0 == 0)
138             d0 = 1 / d0;        /* Divide intentionally by zero.  */

GCC 6.3 and older lowers it to a software division call (__sdivsi3_i4i)
while GCC 7.0 with -fisolate-erroneous-paths-dereference found the
undefined behaviour and transform to a trap and subsequent abort call.

So I think we have some options:

  1. Revert the patch and make SH toolchain compile with
     -fno-isolate-erroneous-paths-dereference (there is no need for
     add -fno-isolate-erroneous-paths-attribute).

  2. Build divdi3 for SH explicit with -fno-isolate-erroneous-paths-attribute.

  3. Port libgcc r205444 with add an __udivmoddi4 implementation for
     architectures that do not have division instruction (which does not
     generate a trap for division by 0).

I would prefer either 2 or 3.
Joseph Myers March 14, 2017, 6:09 p.m. UTC | #6 
On Tue, 14 Mar 2017, Adhemerval Zanella wrote:

> So I think we have some options:
> 
>   1. Revert the patch and make SH toolchain compile with
>      -fno-isolate-erroneous-paths-dereference (there is no need for
>      add -fno-isolate-erroneous-paths-attribute).
> 
>   2. Build divdi3 for SH explicit with -fno-isolate-erroneous-paths-attribute.
> 
>   3. Port libgcc r205444 with add an __udivmoddi4 implementation for
>      architectures that do not have division instruction (which does not
>      generate a trap for division by 0).
> 
> I would prefer either 2 or 3.

This code is present in glibc purely for compat symbols, which are only 
exported on a limited number of architectures, which do not include SH.  
Thus, I'd favour arranging for the code only to be built at all for those 
architectures (i386 m68k powerpc32 s390-32; ia64 also exports these 
symbols, but from a separate implementation), and not for any other 32-bit 
architecture (and if such architectures need these functions in glibc, 
they will get them from libgcc.a).  That would of course need 
build-many-glibcs tests to make sure it doesn't break anything.
Adhemerval Zanella March 14, 2017, 7:55 p.m. UTC | #7 
On 14/03/2017 15:09, Joseph Myers wrote:
> On Tue, 14 Mar 2017, Adhemerval Zanella wrote:
> 
>> So I think we have some options:
>>
>>   1. Revert the patch and make SH toolchain compile with
>>      -fno-isolate-erroneous-paths-dereference (there is no need for
>>      add -fno-isolate-erroneous-paths-attribute).
>>
>>   2. Build divdi3 for SH explicit with -fno-isolate-erroneous-paths-attribute.
>>
>>   3. Port libgcc r205444 with add an __udivmoddi4 implementation for
>>      architectures that do not have division instruction (which does not
>>      generate a trap for division by 0).
>>
>> I would prefer either 2 or 3.
> 
> This code is present in glibc purely for compat symbols, which are only 
> exported on a limited number of architectures, which do not include SH.  
> Thus, I'd favour arranging for the code only to be built at all for those 
> architectures (i386 m68k powerpc32 s390-32; ia64 also exports these 
> symbols, but from a separate implementation), and not for any other 32-bit 
> architecture (and if such architectures need these functions in glibc, 
> they will get them from libgcc.a).  That would of course need 
> build-many-glibcs tests to make sure it doesn't break anything.
> 

Right, I will follow this idea then.
Joseph Myers March 14, 2017, 10:04 p.m. UTC | #8 
On Tue, 14 Mar 2017, Adhemerval Zanella wrote:

> > This code is present in glibc purely for compat symbols, which are only 
> > exported on a limited number of architectures, which do not include SH.  
> > Thus, I'd favour arranging for the code only to be built at all for those 
> > architectures (i386 m68k powerpc32 s390-32; ia64 also exports these 
> > symbols, but from a separate implementation), and not for any other 32-bit 
> > architecture (and if such architectures need these functions in glibc, 
> > they will get them from libgcc.a).  That would of course need 
> > build-many-glibcs tests to make sure it doesn't break anything.
> > 
> 
> Right, I will follow this idea then.

Note that this code goes along with the symbol-hacks.h code that redirects 
__divdi3 to __divdi3_internal etc. - that code will also need to be 
disabled except for the four architectures that actually export code from 
divdi3.c.
Adhemerval Zanella March 15, 2017, 2:23 a.m. UTC | #9 
On 14/03/2017 19:04, Joseph Myers wrote:
> On Tue, 14 Mar 2017, Adhemerval Zanella wrote:
> 
>>> This code is present in glibc purely for compat symbols, which are only 
>>> exported on a limited number of architectures, which do not include SH.  
>>> Thus, I'd favour arranging for the code only to be built at all for those 
>>> architectures (i386 m68k powerpc32 s390-32; ia64 also exports these 
>>> symbols, but from a separate implementation), and not for any other 32-bit 
>>> architecture (and if such architectures need these functions in glibc, 
>>> they will get them from libgcc.a).  That would of course need 
>>> build-many-glibcs tests to make sure it doesn't break anything.
>>>
>>
>> Right, I will follow this idea then.
> 
> Note that this code goes along with the symbol-hacks.h code that redirects 
> __divdi3 to __divdi3_internal etc. - that code will also need to be 
> disabled except for the four architectures that actually export code from 
> divdi3.c.
> 

Yes, this very issue showed itself on a architecture that is not suppose
to export these symbols.
diff mbox

Patch

diff --git a/scripts/build-many-glibcs.py b/scripts/build-many-glibcs.py
index 4330f7f..333ec7f 100755
--- a/scripts/build-many-glibcs.py
+++ b/scripts/build-many-glibcs.py
@@ -161,7 +161,8 @@  class Context(object):
         """Add all known glibc build configurations."""
         # On architectures missing __builtin_trap support, these
         # options may be needed as a workaround; see
-        # <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216> for SH.
+        # <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216> for SH
+        # (although since d40dbe7 SH does not generate trap instruction).
         no_isolate = ('-fno-isolate-erroneous-paths-dereference'
                       ' -fno-isolate-erroneous-paths-attribute')
         self.add_config(arch='aarch64',
@@ -337,31 +338,23 @@  class Context(object):
                         glibcs=[{},
                                 {'arch': 's390', 'ccopts': '-m31'}])
         self.add_config(arch='sh3',
-                        os_name='linux-gnu',
-                        glibcs=[{'ccopts': no_isolate}])
+                        os_name='linux-gnu')
         self.add_config(arch='sh3eb',
-                        os_name='linux-gnu',
-                        glibcs=[{'ccopts': no_isolate}])
+                        os_name='linux-gnu')
         self.add_config(arch='sh4',
-                        os_name='linux-gnu',
-                        glibcs=[{'ccopts': no_isolate}])
+                        os_name='linux-gnu')
         self.add_config(arch='sh4eb',
-                        os_name='linux-gnu',
-                        glibcs=[{'ccopts': no_isolate}])
+                        os_name='linux-gnu')
         self.add_config(arch='sh4',
                         os_name='linux-gnu',
                         variant='soft',
                         gcc_cfg=['--without-fp'],
-                        glibcs=[{'variant': 'soft',
-                                 'cfg': ['--without-fp'],
-                                 'ccopts': no_isolate}])
+                        glibcs=[{'variant': 'soft', 'cfg': ['--without-fp']}])
         self.add_config(arch='sh4eb',
                         os_name='linux-gnu',
                         variant='soft',
                         gcc_cfg=['--without-fp'],
-                        glibcs=[{'variant': 'soft',
-                                 'cfg': ['--without-fp'],
-                                 'ccopts': no_isolate}])
+                        glibcs=[{'variant': 'soft', 'cfg': ['--without-fp']}])
         self.add_config(arch='sparc64',
                         os_name='linux-gnu',
                         glibcs=[{},