Re: [PATCH 1/4] Add mkoffload for Intel MIC

[Ilya Verbin <iverbin@gmail.com>]

2014-10-22 1:55 GMT+04:00 Joseph S. Myers <joseph@codesourcery.com>:
> On Tue, 21 Oct 2014, Ilya Verbin wrote:
>
>> +#include <libgen.h>
>> +#include "config.h"
>> +#include "system.h"
>
> You should never include system headers before config.h because config.h
> may define feature test macros such as _FILE_OFFSET_BITS=64 that are
> ineffective if defined after any system header is included.

I will fix this.

> I don't see anything restricting this program to being built for GNU
> *hosts*.  Thus, it needs to be portable (to different hosts; obviously
> it's target-architecture-specific) rather than relying on glibc
> interfaces.  (Providing appropriate functions in libiberty is of course an
> option; thus, freely using obstacks is fine because they're in libiberty.)

This mkoffload is expected to be built only with the offload compiler,
which is expected to be configured with
'--enable-as-accelerator-for=... --host=x86_64-*-linux-gnu
--target=x86_64-*-linux-gnu'.
Without $enable_as_accelerator it wouldn't be built. I will add more
restrictions to config.gcc.

>> +#include <libgomp_target.h>
>
> Where does this header come from?

It comes from libgomp directory (
https://gcc.gnu.org/ml/gcc-patches/2014-10/msg00475.html ).

>> +      nextval = strchrnul (curval, ':');
>
> I don't think strchrnul is portable (unless added to libiberty).

It seems that there is no need to make this mkoffload so portable.

>> +  if (!host_compiler)
>> +    fatal_error ("COLLECT_GCC must be set.");
>
> Diagnostics should not end with ".".

I will fix all fatal_errors.

Thanks,
  -- Ilya