| Message ID | D7EFF377C0894A47B63545C223A166A531B9B1D4@MBX-S2.rwth-ad.de |
|---|---|
| State | New |
| Headers | show |
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
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
> -----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
> -----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
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
> -----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
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.
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
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?
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
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;