diff mbox

build-many-glibcs: Remove no_isolate from SH config

Message ID 1489403315-6856-1-git-send-email-adhemerval.zanella@linaro.org
State Accepted
Commit c89721e25d609ec4f3366a3956b2f3e5acd76e77
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(-)

-- 
2.7.4

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.

-- 
Joseph S. Myers
joseph@codesourcery.com
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

-- 
Joseph S. Myers
joseph@codesourcery.com
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.

-- 
Joseph S. Myers
joseph@codesourcery.com
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.

-- 
Joseph S. Myers
joseph@codesourcery.com
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=[{},