[1/4] Add mkoffload for Intel MIC

Hi!

On Wed, 22 Oct 2014 22:57:01 +0400, Ilya Verbin <iverbin@gmail.com> wrote:
> On 22 Oct 09:57, Jakub Jelinek wrote:
> > On Wed, Oct 22, 2014 at 02:30:44AM +0400, Ilya Verbin wrote:
> > > +  obstack_init (&argv_obstack);
> > > +  obstack_ptr_grow (&argv_obstack, "objcopy");
> > > +  obstack_ptr_grow (&argv_obstack, target_so_filename);
> > > +  obstack_ptr_grow (&argv_obstack, "--redefine-sym");
> > > +  obstack_ptr_grow (&argv_obstack, opt_for_objcopy[0]);
> > > +  obstack_ptr_grow (&argv_obstack, "--redefine-sym");
> > > +  obstack_ptr_grow (&argv_obstack, opt_for_objcopy[1]);
> > > +  obstack_ptr_grow (&argv_obstack, "--redefine-sym");
> > > +  obstack_ptr_grow (&argv_obstack, opt_for_objcopy[2]);
> > > +  obstack_ptr_grow (&argv_obstack, NULL);
> > > +  new_argv = XOBFINISH (&argv_obstack, char **);
> > 
> > Why do you use an obstack for an array of pointers where you know
> > you have exactly 9 pointers?  Wouldn't
> >   char *new_argv[9];
> > and just pointer assignments be better?
> 
> Yes, done.
> 
> > > +  /* Perform partial linking for the target image and host side descriptor.
> > > +     As a result we'll get a finalized object file with all offload data.  */
> > > +  struct obstack argv_obstack;
> > > +  obstack_init (&argv_obstack);
> > > +  obstack_ptr_grow (&argv_obstack, "ld");
> > > +  if (target_ilp32)
> > > +    {
> > > +      obstack_ptr_grow (&argv_obstack, "-m");
> > > +      obstack_ptr_grow (&argv_obstack, "elf_i386");
> > > +    }
> > > +  obstack_ptr_grow (&argv_obstack, "-r");
> > > +  obstack_ptr_grow (&argv_obstack, host_descr_filename);
> > > +  obstack_ptr_grow (&argv_obstack, target_so_filename);
> > > +  obstack_ptr_grow (&argv_obstack, "-o");
> > > +  obstack_ptr_grow (&argv_obstack, out_obj_filename);
> > > +  obstack_ptr_grow (&argv_obstack, NULL);
> > > +  char **new_argv = XOBFINISH (&argv_obstack, char **);
> > 
> > Similarly (well, here it is not constant, still, you know small upper bound
> > and can just use some int index you ++ in each assignment.
> 
> Done.
> 
> > > +  /* Run objcopy on the resultant object file to localize generated symbols
> > > +     to avoid conflicting between different DSO and an executable.  */
> > > +  obstack_init (&argv_obstack);
> > > +  obstack_ptr_grow (&argv_obstack, "objcopy");
> > > +  obstack_ptr_grow (&argv_obstack, "-L");
> > > +  obstack_ptr_grow (&argv_obstack, symbols[0]);
> > > +  obstack_ptr_grow (&argv_obstack, "-L");
> > > +  obstack_ptr_grow (&argv_obstack, symbols[1]);
> > > +  obstack_ptr_grow (&argv_obstack, "-L");
> > > +  obstack_ptr_grow (&argv_obstack, symbols[2]);
> > > +  obstack_ptr_grow (&argv_obstack, out_obj_filename);
> > > +  obstack_ptr_grow (&argv_obstack, NULL);
> > > +  new_argv = XOBFINISH (&argv_obstack, char **);
> > > +  fork_execute (new_argv[0], new_argv, false);
> > > +  obstack_free (&argv_obstack, NULL);
> > 
> > Likewise.
> 
> Done.

After approval for "Use gcc/coretypes.h:enum offload_abi in mkoffloads",
<http://news.gmane.org/find-root.php?message_id=%3C87vbavuft0.fsf%40kepler.schwinge.homeip.net%3E>,
I'd like to commit the following refactoring patch to trunk, in
preparation for another change:

commit 91fbe15ce2e539a4017f65cc167b362a4b4e4553
Author: Thomas Schwinge <thomas@codesourcery.com>
Date:   Tue Aug 4 14:06:39 2015 +0200

    Refactor intelmic-mkoffload.c argv building
    
    	gcc/
    	* config/i386/intelmic-mkoffload.c (generate_host_descr_file)
    	(prepare_target_image, main): Refactor argv building.
---
 gcc/config/i386/intelmic-mkoffload.c |   88 +++++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 34 deletions(-)



Grüße,
 Thomas

On 09/28/2015 10:26 AM, Thomas Schwinge wrote:
> -  objcopy_argv[8] = NULL;
> +  objcopy_argv[objcopy_argc++] = NULL;
> +  gcc_checking_assert (objcopy_argc <= OBJCOPY_ARGC_MAX);

On its own this is not an improvement - you're trading a compile time 
error for a runtime error. So, what is the other change this is 
preparing for?


Bernd

On 09/28/2015 12:03 PM, Bernd Schmidt wrote:
> On 09/28/2015 10:26 AM, Thomas Schwinge wrote:
>> -  objcopy_argv[8] = NULL;
>> +  objcopy_argv[objcopy_argc++] = NULL;
>> +  gcc_checking_assert (objcopy_argc <= OBJCOPY_ARGC_MAX);
>
> On its own this is not an improvement - you're trading a compile time
> error for a runtime error. So, what is the other change this is
> preparing for?

Ok, I now see the other patch. But I also see that other code in the 
same file and in the nvptx mkoffload is using the obstack_ptr_grow 
method to build argv arrays, I think that would be preferrable to this.


Bernd

On Mon, Sep 28, 2015 at 12:09:19 +0200, Bernd Schmidt wrote:
> On 09/28/2015 12:03 PM, Bernd Schmidt wrote:
> >On 09/28/2015 10:26 AM, Thomas Schwinge wrote:
> >>-  objcopy_argv[8] = NULL;
> >>+  objcopy_argv[objcopy_argc++] = NULL;
> >>+  gcc_checking_assert (objcopy_argc <= OBJCOPY_ARGC_MAX);
> >
> >On its own this is not an improvement - you're trading a compile time
> >error for a runtime error. So, what is the other change this is
> >preparing for?
> 
> Ok, I now see the other patch. But I also see that other code in the same
> file and in the nvptx mkoffload is using the obstack_ptr_grow method to
> build argv arrays, I think that would be preferrable to this.

I've removed obstack_ptr_grow for arrays with known sizes after this review:
https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02210.html

  -- Ilya

On 09/28/2015 01:25 PM, Ilya Verbin wrote:
> On Mon, Sep 28, 2015 at 12:09:19 +0200, Bernd Schmidt wrote:
>> On 09/28/2015 12:03 PM, Bernd Schmidt wrote:
>>> On 09/28/2015 10:26 AM, Thomas Schwinge wrote:
>>>> -  objcopy_argv[8] = NULL;
>>>> +  objcopy_argv[objcopy_argc++] = NULL;
>>>> +  gcc_checking_assert (objcopy_argc <= OBJCOPY_ARGC_MAX);
>>>
>>> On its own this is not an improvement - you're trading a compile time
>>> error for a runtime error. So, what is the other change this is
>>> preparing for?
>>
>> Ok, I now see the other patch. But I also see that other code in the same
>> file and in the nvptx mkoffload is using the obstack_ptr_grow method to
>> build argv arrays, I think that would be preferrable to this.
>
> I've removed obstack_ptr_grow for arrays with known sizes after this review:
> https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02210.html

That's unfortunate, I think that made the code less future-proof. IMO we 
should revert to the obstack method especially if Thomas -v patch goes in.


Bernd

On Mon, Sep 28, 2015 at 01:27:32PM +0200, Bernd Schmidt wrote:
> >I've removed obstack_ptr_grow for arrays with known sizes after this review:
> >https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02210.html
> 
> That's unfortunate, I think that made the code less future-proof. IMO we
> should revert to the obstack method especially if Thomas -v patch goes in.

Why?  If the number of arguments is bound by a small constant, using
automatic fixed size array is certainly more efficient, and I really don't
see it as less readable or maintainable.

	Jakub

On 09/28/2015 02:00 PM, Jakub Jelinek wrote:
> On Mon, Sep 28, 2015 at 01:27:32PM +0200, Bernd Schmidt wrote:
>>> I've removed obstack_ptr_grow for arrays with known sizes after this review:
>>> https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02210.html
>>
>> That's unfortunate, I think that made the code less future-proof. IMO we
>> should revert to the obstack method especially if Thomas -v patch goes in.
>
> Why?  If the number of arguments is bound by a small constant, using
> automatic fixed size array is certainly more efficient, and I really don't
> see it as less readable or maintainable.

The code becomes harder to modify, with more room for error, and you no 
longer have consistency in how you build argv arrays within the same 
file. The obstack method is pretty much foolproof and doesn't even 
remotely allow for the possibility of a buffer overflow, and adding new 
arguments, even conditionally, is entirely trivial. Efficiency is really 
not an issue for building arguments compared to the cost of executing 
another binary.


Bernd

On Mon, Sep 28, 2015 at 2:05 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 09/28/2015 02:00 PM, Jakub Jelinek wrote:
>>
>> On Mon, Sep 28, 2015 at 01:27:32PM +0200, Bernd Schmidt wrote:
>>>>
>>>> I've removed obstack_ptr_grow for arrays with known sizes after this
>>>> review:
>>>> https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02210.html
>>>
>>>
>>> That's unfortunate, I think that made the code less future-proof. IMO we
>>> should revert to the obstack method especially if Thomas -v patch goes
>>> in.
>>
>>
>> Why?  If the number of arguments is bound by a small constant, using
>> automatic fixed size array is certainly more efficient, and I really don't
>> see it as less readable or maintainable.
>
>
> The code becomes harder to modify, with more room for error, and you no
> longer have consistency in how you build argv arrays within the same file.
> The obstack method is pretty much foolproof and doesn't even remotely allow
> for the possibility of a buffer overflow, and adding new arguments, even
> conditionally, is entirely trivial. Efficiency is really not an issue for
> building arguments compared to the cost of executing another binary.

I agree that obstacks are better here.  Efficiency shouldn't matter here.
But we're in C++ now so can't we statically construct the array with
sth like

const char *new_argv[] = { "objcopy", ... };

?  Thus have the compiler figure out the number of args.  That would work
for me as well.

Richard.

>
> Bernd

On 09/29/2015 12:29 PM, Richard Biener wrote:
> I agree that obstacks are better here.  Efficiency shouldn't matter here.
> But we're in C++ now so can't we statically construct the array with
> sth like
>
> const char *new_argv[] = { "objcopy", ... };
>
> ?  Thus have the compiler figure out the number of args.  That would work
> for me as well.

The issue is that the code is about to be changed to conditionally pass 
certain arguments ("-v"), so you no longer have a fixed arglist.


Bernd