Fix intelmic-mkoffload.c if the temp path contains a '-'

Fix intelmic-mkoffload.c if the temp path contains a '-'

[Hahnfeld, Jonas]

[-- Attachment #1.1: Type: text/plain, Size: 703 bytes --]

Hi all,

during my test of OpenMP 4.0 offloading features I have found a bug in
intelmic-mkoffload.c when the temp path contains a '-'.
objcopy will in this case replace it with a '_' which wasn't reflected in
the original code and resulted in a link error of the symbols
'__offload_image_intelmic_start' and '__offload_image_intelmic_end'.

This is my first contribution, so just reply if anything is wrong and I'll
happily fix it.

Greetings,
Jonas

--
Jonas Hahnfeld, MATSE-Auszubildender

IT Center
Group: High Performance Computing
Division: Computational Science and Engineering
RWTH Aachen University
Seffenter Weg 23
D 52074  Aachen (Germany)
Hahnfeld@itc.rwth-aachen.de
www.itc.rwth-aachen.de


[-- Attachment #1.2: Fix-intelmic-mkoffload.c-if-the-temp-path-contains-a.patch --]
[-- Type: application/octet-stream, Size: 1050 bytes --]

From 884b6199179e7a604474bc6a828a6861d3ff4501 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld <Hahnfeld@itc.rwth-aachen.de>
Date: Thu, 20 Aug 2015 12:13:55 +0200
Subject: [PATCH] Fix intelmic-mkoffload.c if the temp path contains a '-'

2015-08-20  Jonas Hahnfeld  <Hahnfeld@itc.rwth-aachen.de>

	* intelmic-mkoffload.c (prepare_target_image): Fix if the temp path
	contains a '-'.
---
 gcc/config/i386/intelmic-mkoffload.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gcc/config/i386/intelmic-mkoffload.c b/gcc/config/i386/intelmic-mkoffload.c
index ca15868..c9327cf 100644
--- a/gcc/config/i386/intelmic-mkoffload.c
+++ b/gcc/config/i386/intelmic-mkoffload.c
@@ -460,7 +460,7 @@ prepare_target_image (const char *target_compiler, int argc, char **argv)
   for (size_t i = 0; i <= symbol_name_len; i++)
     {
       char c = target_so_filename[i];
-      if ((c == '/') || (c == '.'))
+      if ((c == '/') || (c == '.') || (c == '-'))
 	c = '_';
       symbol_name[i] = c;
     }
-- 
1.7.1


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5868 bytes --]

Re: Fix intelmic-mkoffload.c if the temp path contains a '-'

[Jakub Jelinek]

On Mon, Sep 07, 2015 at 05:46:12PM +0300, Ilya Verbin wrote:
> On Sat, Sep 05, 2015 at 00:45:36 +0300, Ilya Verbin wrote:
> > 2015-09-04 22:27 GMT+03:00 Mike Stump <mikestump@comcast.net>:
> > > On Sep 4, 2015, at 4:10 AM, Hahnfeld, Jonas <Hahnfeld@itc.rwth-aachen.de> wrote:
> > >>>>>    * intelmic-mkoffload.c (prepare_target_image): Fix if the temp path
> > >>>>>    contains a '-‘.
> > >
> > > So, out of curiosity, did you test all characters other than null?  If - doesn’t work, there is a good chance that no such test has been done, and there is a small hoard of bugs, all the same in there.
> > 
> > Good point.  Objcopy in bfd/binary.c creates symbol names this way:
> > 
> >   for (p = buf; *p; p++)
> >     if (! ISALNUM (*p))
> >       *p = '_';
> > 
> > We should do the same in intelmic-mkoffload.c.  I will prepare a patch.
> 
> gcc/
> 	* config/i386/intelmic-mkoffload.c (prepare_target_image): Handle all
> 	non-alphanumeric characters in the symbol name.
> 
> Regtested on x86_64-linux.  OK for trunk?  OK for gcc-5-branch?

Ok for both.

	Jakub

Re: Fix intelmic-mkoffload.c if the temp path contains a '-'

[Ilya Verbin]

On Sat, Sep 05, 2015 at 00:45:36 +0300, Ilya Verbin wrote:
> 2015-09-04 22:27 GMT+03:00 Mike Stump <mikestump@comcast.net>:
> > On Sep 4, 2015, at 4:10 AM, Hahnfeld, Jonas <Hahnfeld@itc.rwth-aachen.de> wrote:
> >>>>>    * intelmic-mkoffload.c (prepare_target_image): Fix if the temp path
> >>>>>    contains a '-‘.
> >
> > So, out of curiosity, did you test all characters other than null?  If - doesn’t work, there is a good chance that no such test has been done, and there is a small hoard of bugs, all the same in there.
> 
> Good point.  Objcopy in bfd/binary.c creates symbol names this way:
> 
>   for (p = buf; *p; p++)
>     if (! ISALNUM (*p))
>       *p = '_';
> 
> We should do the same in intelmic-mkoffload.c.  I will prepare a patch.

gcc/
	* config/i386/intelmic-mkoffload.c (prepare_target_image): Handle all
	non-alphanumeric characters in the symbol name.

Regtested on x86_64-linux.  OK for trunk?  OK for gcc-5-branch?


diff --git a/gcc/config/i386/intelmic-mkoffload.c b/gcc/config/i386/intelmic-mkoffload.c
index 49e99e8..4a7812c 100644
--- a/gcc/config/i386/intelmic-mkoffload.c
+++ b/gcc/config/i386/intelmic-mkoffload.c
@@ -453,17 +453,18 @@ prepare_target_image (const char *target_compiler, int argc, char **argv)
   fork_execute (objcopy_argv[0], CONST_CAST (char **, objcopy_argv), false);
 
   /* Objcopy has created symbols, containing the input file name with
-     special characters replaced with '_'.  We are going to rename these
-     new symbols.  */
+     non-alphanumeric characters replaced by underscores.
+     We are going to rename these new symbols.  */
   size_t symbol_name_len = strlen (target_so_filename);
   char *symbol_name = XALLOCAVEC (char, symbol_name_len + 1);
-  for (size_t i = 0; i <= symbol_name_len; i++)
+  for (size_t i = 0; i < symbol_name_len; i++)
     {
       char c = target_so_filename[i];
-      if (c == '/' || c == '.' || c == '-')
+      if (!ISALNUM (c))
 	c = '_';
       symbol_name[i] = c;
     }
+  symbol_name[symbol_name_len] = '\0';
 
   char *opt_for_objcopy[3];
   opt_for_objcopy[0] = XALLOCAVEC (char, sizeof ("_binary__start=")


  -- Ilya

Re: Fix intelmic-mkoffload.c if the temp path contains a '-'

[Ilya Verbin]

2015-09-05 1:50 GMT+03:00 Mike Stump <mikestump@comcast.net>:
>
> On Sep 4, 2015, at 2:45 PM, Ilya Verbin <iverbin@gmail.com> wrote:
>
>> 2015-09-04 22:27 GMT+03:00 Mike Stump <mikestump@comcast.net>:
>>> On Sep 4, 2015, at 4:10 AM, Hahnfeld, Jonas <Hahnfeld@itc.rwth-aachen.de> wrote:
>>>>>>>   * intelmic-mkoffload.c (prepare_target_image): Fix if the temp path
>>>>>>>   contains a '-‘.
>>>
>>> So, out of curiosity, did you test all characters other than null?  If - doesn’t work, there is a good chance that no such test has been done, and there is a small hoard of bugs, all the same in there.
>>
>> Good point.  Objcopy in bfd/binary.c creates symbol names this way:
>>
>>  for (p = buf; *p; p++)
>>    if (! ISALNUM (*p))
>>      *p = '_’;
>
> So, this code can’t be the code in question, as - is ! ALNUM, and then should have been a _, meaning ‘-‘ would not have to be handled.  Since the code is handling ‘-‘, this can’t be the code in question?

When we run "objcopy <args> /tmp-1/liba.so", this code in bfd/binary.c
creates a symbol called "_binary__tmp_1_liba_so_start".  Then
intelmic-mkoffload.c wants to rename it to
"__offload_image_intelmic_start".  To do this, it prepares a string
"--redefine-sym
_binary__tmp_1_liba_so_start=__offload_image_intelmic_start",
therefore it should replace all ! ISALNUM chars in the name, as
objcopy does.

  -- Ilya

Re: Fix intelmic-mkoffload.c if the temp path contains a '-'

[Mike Stump]

On Sep 4, 2015, at 2:45 PM, Ilya Verbin <iverbin@gmail.com> wrote:

> 2015-09-04 22:27 GMT+03:00 Mike Stump <mikestump@comcast.net>:
>> On Sep 4, 2015, at 4:10 AM, Hahnfeld, Jonas <Hahnfeld@itc.rwth-aachen.de> wrote:
>>>>>>   * intelmic-mkoffload.c (prepare_target_image): Fix if the temp path
>>>>>>   contains a '-‘.
>> 
>> So, out of curiosity, did you test all characters other than null?  If - doesn’t work, there is a good chance that no such test has been done, and there is a small hoard of bugs, all the same in there.
> 
> Good point.  Objcopy in bfd/binary.c creates symbol names this way:
> 
>  for (p = buf; *p; p++)
>    if (! ISALNUM (*p))
>      *p = '_’;

So, this code can’t be the code in question, as - is ! ALNUM, and then should have been a _, meaning ‘-‘ would not have to be handled.  Since the code is handling ‘-‘, this can’t be the code in question?

Re: Fix intelmic-mkoffload.c if the temp path contains a '-'

[Ilya Verbin]

2015-09-04 22:27 GMT+03:00 Mike Stump <mikestump@comcast.net>:
> On Sep 4, 2015, at 4:10 AM, Hahnfeld, Jonas <Hahnfeld@itc.rwth-aachen.de> wrote:
>>>>>    * intelmic-mkoffload.c (prepare_target_image): Fix if the temp path
>>>>>    contains a '-‘.
>
> So, out of curiosity, did you test all characters other than null?  If - doesn’t work, there is a good chance that no such test has been done, and there is a small hoard of bugs, all the same in there.

Good point.  Objcopy in bfd/binary.c creates symbol names this way:

  for (p = buf; *p; p++)
    if (! ISALNUM (*p))
      *p = '_';

We should do the same in intelmic-mkoffload.c.  I will prepare a patch.

  -- Ilya

Re: Fix intelmic-mkoffload.c if the temp path contains a '-'

[Mike Stump]

On Sep 4, 2015, at 4:10 AM, Hahnfeld, Jonas <Hahnfeld@itc.rwth-aachen.de> wrote:
>>>> 	* intelmic-mkoffload.c (prepare_target_image): Fix if the temp path
>>>> 	contains a '-‘.

So, out of curiosity, did you test all characters other than null?  If - doesn’t work, there is a good chance that no such test has been done, and there is a small hoard of bugs, all the same in there.

RE: Fix intelmic-mkoffload.c if the temp path contains a '-'

[Hahnfeld, Jonas]

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

> -----Original Message-----
> From: Ilya Verbin [mailto:iverbin@gmail.com]
> Sent: Friday, September 04, 2015 1:23 PM
> To: Hahnfeld, Jonas; Jakub Jelinek
> Cc: gcc-patches@gcc.gnu.org; Kirill Yukhin
> Subject: Re: Fix intelmic-mkoffload.c if the temp path contains a '-'
> 
> On Fri, Sep 04, 2015 at 11:10:13 +0000, Hahnfeld, Jonas wrote:
> > *ping*
> > I don't have write access to the repository...
> 
> I've committed it to trunk:
> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=227489
> 
> What about gcc-5-branch?
> 
>   -- Ilya

Thanks!
The bug exists in GCC 5.x as well so the fix should be included in 5.3 if
there will be such a version...

Greetings,
Jonas

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5868 bytes --]

Re: Fix intelmic-mkoffload.c if the temp path contains a '-'

[Ilya Verbin]

On Fri, Sep 04, 2015 at 11:10:13 +0000, Hahnfeld, Jonas wrote:
> *ping*
> I don't have write access to the repository...

I've committed it to trunk: https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=227489

What about gcc-5-branch?

  -- Ilya

RE: Fix intelmic-mkoffload.c if the temp path contains a '-'

[Hahnfeld, Jonas]

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

> -----Original Message-----
> From: Hahnfeld, Jonas
> Sent: Tuesday, September 01, 2015 1:54 PM
> To: 'Jakub Jelinek'
> Cc: gcc-patches@gcc.gnu.org; Ilya Verbin; Kirill Yukhin
> Subject: RE: Fix intelmic-mkoffload.c if the temp path contains a '-'
> 
> > -----Original Message-----
> > From: Jakub Jelinek [mailto:jakub@redhat.com]
> > Sent: Tuesday, September 01, 2015 1:50 PM
> > To: Hahnfeld, Jonas
> > Cc: gcc-patches@gcc.gnu.org; Ilya Verbin; Kirill Yukhin
> > Subject: Re: Fix intelmic-mkoffload.c if the temp path contains a '-'
> >
> > On Tue, Sep 01, 2015 at 11:35:15AM +0000, Hahnfeld, Jonas wrote:
> > > >From 884b6199179e7a604474bc6a828a6861d3ff4501 Mon Sep 17
> 00:00:00
> > > >2001
> > > From: Jonas Hahnfeld <Hahnfeld@itc.rwth-aachen.de>
> > > Date: Thu, 20 Aug 2015 12:13:55 +0200
> > > Subject: [PATCH] Fix intelmic-mkoffload.c if the temp path contains a
> '-'
> > >
> > > 2015-08-20  Jonas Hahnfeld  <Hahnfeld@itc.rwth-aachen.de>
> > >
> > > 	* intelmic-mkoffload.c (prepare_target_image): Fix if the temp path
> > > 	contains a '-'.
> > > ---
> > >  gcc/config/i386/intelmic-mkoffload.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/gcc/config/i386/intelmic-mkoffload.c
> > > b/gcc/config/i386/intelmic-mkoffload.c
> > > index ca15868..c9327cf 100644
> > > --- a/gcc/config/i386/intelmic-mkoffload.c
> > > +++ b/gcc/config/i386/intelmic-mkoffload.c
> > > @@ -460,7 +460,7 @@ prepare_target_image (const char
> > *target_compiler,
> > > int argc, char **argv)
> > >    for (size_t i = 0; i <= symbol_name_len; i++)
> > >      {
> > >        char c = target_so_filename[i];
> > > -      if ((c == '/') || (c == '.'))
> > > +      if ((c == '/') || (c == '.') || (c == '-'))
> >
> > The ()s around the comparisons are unnecessary, but it is preexisting,
so
> the
> > fix is ok for trunk with or without the removal of those ()s.
> >
> > 	Jakub
> 
> Could you commit the patch for me as I don't have commit access?
> And also consider backporting this trivial fix for 'gcc-5-branch'?
> 
> Thanks,
> Jonas

*ping*
I don't have write access to the repository...

Thanks,
Jonas

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5868 bytes --]

RE: Fix intelmic-mkoffload.c if the temp path contains a '-'

[Hahnfeld, Jonas]

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

> -----Original Message-----
> From: Jakub Jelinek [mailto:jakub@redhat.com]
> Sent: Tuesday, September 01, 2015 1:50 PM
> To: Hahnfeld, Jonas
> Cc: gcc-patches@gcc.gnu.org; Ilya Verbin; Kirill Yukhin
> Subject: Re: Fix intelmic-mkoffload.c if the temp path contains a '-'
> 
> On Tue, Sep 01, 2015 at 11:35:15AM +0000, Hahnfeld, Jonas wrote:
> > >From 884b6199179e7a604474bc6a828a6861d3ff4501 Mon Sep 17 00:00:00
> > >2001
> > From: Jonas Hahnfeld <Hahnfeld@itc.rwth-aachen.de>
> > Date: Thu, 20 Aug 2015 12:13:55 +0200
> > Subject: [PATCH] Fix intelmic-mkoffload.c if the temp path contains a
'-'
> >
> > 2015-08-20  Jonas Hahnfeld  <Hahnfeld@itc.rwth-aachen.de>
> >
> > 	* intelmic-mkoffload.c (prepare_target_image): Fix if the temp path
> > 	contains a '-'.
> > ---
> >  gcc/config/i386/intelmic-mkoffload.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/gcc/config/i386/intelmic-mkoffload.c
> > b/gcc/config/i386/intelmic-mkoffload.c
> > index ca15868..c9327cf 100644
> > --- a/gcc/config/i386/intelmic-mkoffload.c
> > +++ b/gcc/config/i386/intelmic-mkoffload.c
> > @@ -460,7 +460,7 @@ prepare_target_image (const char
> *target_compiler,
> > int argc, char **argv)
> >    for (size_t i = 0; i <= symbol_name_len; i++)
> >      {
> >        char c = target_so_filename[i];
> > -      if ((c == '/') || (c == '.'))
> > +      if ((c == '/') || (c == '.') || (c == '-'))
> 
> The ()s around the comparisons are unnecessary, but it is preexisting, so
the
> fix is ok for trunk with or without the removal of those ()s.
> 
> 	Jakub

Could you commit the patch for me as I don't have commit access?
And also consider backporting this trivial fix for 'gcc-5-branch'?

Thanks,
Jonas

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5868 bytes --]

Re: Fix intelmic-mkoffload.c if the temp path contains a '-'

[Jakub Jelinek]

On Tue, Sep 01, 2015 at 11:35:15AM +0000, Hahnfeld, Jonas wrote:
> >From 884b6199179e7a604474bc6a828a6861d3ff4501 Mon Sep 17 00:00:00 2001
> From: Jonas Hahnfeld <Hahnfeld@itc.rwth-aachen.de>
> Date: Thu, 20 Aug 2015 12:13:55 +0200
> Subject: [PATCH] Fix intelmic-mkoffload.c if the temp path contains a '-'
> 
> 2015-08-20  Jonas Hahnfeld  <Hahnfeld@itc.rwth-aachen.de>
> 
> 	* intelmic-mkoffload.c (prepare_target_image): Fix if the temp path
> 	contains a '-'.
> ---
>  gcc/config/i386/intelmic-mkoffload.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/gcc/config/i386/intelmic-mkoffload.c
> b/gcc/config/i386/intelmic-mkoffload.c
> index ca15868..c9327cf 100644
> --- a/gcc/config/i386/intelmic-mkoffload.c
> +++ b/gcc/config/i386/intelmic-mkoffload.c
> @@ -460,7 +460,7 @@ prepare_target_image (const char *target_compiler, int
> argc, char **argv)
>    for (size_t i = 0; i <= symbol_name_len; i++)
>      {
>        char c = target_so_filename[i];
> -      if ((c == '/') || (c == '.'))
> +      if ((c == '/') || (c == '.') || (c == '-'))

The ()s around the comparisons are unnecessary, but it is preexisting,
so the fix is ok for trunk with or without the removal of those ()s.

	Jakub

Re: Fix intelmic-mkoffload.c if the temp path contains a '-'

[Ilya Verbin]

On Tue, Sep 01, 2015 at 11:35:15 +0000, Hahnfeld, Jonas wrote:
> > during my test of OpenMP 4.0 offloading features I have found a bug in
> > intelmic-mkoffload.c when the temp path contains a '-'.
> > objcopy will in this case replace it with a '_' which wasn't reflected in
> > the original code and resulted in a link error of the symbols
> > '__offload_image_intelmic_start' and '__offload_image_intelmic_end'.
> > 
> > This is my first contribution, so just reply if anything is wrong and I'll
> > happily fix it.

Thanks!  Please wait for Jakub's approval before checking-in.

  -- Ilya

RE: Fix intelmic-mkoffload.c if the temp path contains a '-'

[Hahnfeld, Jonas]

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

Hopefully CC'ing the right people...

> -----Original Message-----
> From: Hahnfeld, Jonas
> Sent: Thursday, August 20, 2015 12:25 PM
> To: 'gcc-patches@gcc.gnu.org'
> Subject: Fix intelmic-mkoffload.c if the temp path contains a '-'
> 
> Hi all,
> 
> during my test of OpenMP 4.0 offloading features I have found a bug in
> intelmic-mkoffload.c when the temp path contains a '-'.
> objcopy will in this case replace it with a '_' which wasn't reflected in
> the original code and resulted in a link error of the symbols
> '__offload_image_intelmic_start' and '__offload_image_intelmic_end'.
> 
> This is my first contribution, so just reply if anything is wrong and I'll
> happily fix it.
> 
> Greetings,
> Jonas
> 
> --
> Jonas Hahnfeld, MATSE-Auszubildender
> 
> IT Center
> Group: High Performance Computing
> Division: Computational Science and Engineering
> RWTH Aachen University
> Seffenter Weg 23
> D 52074  Aachen (Germany)
> Hahnfeld@itc.rwth-aachen.de
> www.itc.rwth-aachen.de

From git-format-patch:

From 884b6199179e7a604474bc6a828a6861d3ff4501 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld <Hahnfeld@itc.rwth-aachen.de>
Date: Thu, 20 Aug 2015 12:13:55 +0200
Subject: [PATCH] Fix intelmic-mkoffload.c if the temp path contains a '-'

2015-08-20  Jonas Hahnfeld  <Hahnfeld@itc.rwth-aachen.de>

	* intelmic-mkoffload.c (prepare_target_image): Fix if the temp path
	contains a '-'.
---
 gcc/config/i386/intelmic-mkoffload.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gcc/config/i386/intelmic-mkoffload.c
b/gcc/config/i386/intelmic-mkoffload.c
index ca15868..c9327cf 100644
--- a/gcc/config/i386/intelmic-mkoffload.c
+++ b/gcc/config/i386/intelmic-mkoffload.c
@@ -460,7 +460,7 @@ prepare_target_image (const char *target_compiler, int
argc, char **argv)
   for (size_t i = 0; i <= symbol_name_len; i++)
     {
       char c = target_so_filename[i];
-      if ((c == '/') || (c == '.'))
+      if ((c == '/') || (c == '.') || (c == '-'))
 	c = '_';
       symbol_name[i] = c;
     }
-- 
1.7.1


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5868 bytes --]