NaCl: Fix nacl_io library to build with -Wstrict-prototypes

NaCl: Fix nacl_io library to build with -Wstrict-prototypes

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3114
TEST=compile "nacl_io_untrusted" ninja target

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276023

[Sam Clegg, 2014-06-06 23:00:14]

Presumably we should be building nacl_io with this flag then, or at least
building that ninja target as part of out bot runs?

[Mark Seaborn, 2014-06-06 23:12:39]

On 2014/06/06 23:00:14, Sam Clegg wrote:
> Presumably we should be building nacl_io with this flag then, or at least
> building that ninja target as part of out bot runs?

Ah, sorry, this is a dependency for this change:
https://codereview.chromium.org/11415157/, which will just apply the flag
globally for all untrusted C code.

It seemed easier to enable the flag globally (and fix the warnings) than to do
it for NaCl-side code only, assuming I don't hit more instances of the warning. 
It's more correct to use "(void)" anyway.

The Chromium trybot runs do include nacl_io -- chromium_builder_tests depends on
it.

[binji, 2014-06-06 23:32:55]

On 2014/06/06 23:12:39, Mark Seaborn wrote:
> On 2014/06/06 23:00:14, Sam Clegg wrote:
> > Presumably we should be building nacl_io with this flag then, or at least
> > building that ninja target as part of out bot runs?
> 
> Ah, sorry, this is a dependency for this change:
> https://codereview.chromium.org/11415157/, which will just apply the flag
> globally for all untrusted C code.
> 
> It seemed easier to enable the flag globally (and fix the warnings) than to do
> it for NaCl-side code only, assuming I don't hit more instances of the
warning. 
> It's more correct to use "(void)" anyway.
> 
> The Chromium trybot runs do include nacl_io -- chromium_builder_tests depends
on
> it.

Yes, but we typically build nacl_io via the generated Makefiles, not GYP.
Can you add

'CFLAGS': ['-Wstrict-prototypes']

to native_client_sdk/src/libraries/nacl_io/library.dsc?

[Mark Seaborn, 2014-06-06 23:52:13]

On 6 June 2014 16:32, <binji@chromium.org> wrote:

> On 2014/06/06 23:12:39, Mark Seaborn wrote:
>
>> On 2014/06/06 23:00:14, Sam Clegg wrote:
>> > Presumably we should be building nacl_io with this flag then, or at
>> least
>> > building that ninja target as part of out bot runs?
>>
>
>  Ah, sorry, this is a dependency for this change:
>> https://codereview.chromium.org/11415157/, which will just apply the flag
>> globally for all untrusted C code.
>>
>
>  It seemed easier to enable the flag globally (and fix the warnings) than
>> to do
>> it for NaCl-side code only, assuming I don't hit more instances of the
>> warning.
>
>  It's more correct to use "(void)" anyway.
>>
>
>  The Chromium trybot runs do include nacl_io -- chromium_builder_tests
>> depends on
>
>  it.
>>
>
> Yes, but we typically build nacl_io via the generated Makefiles, not GYP.
> Can you add
>
> 'CFLAGS': ['-Wstrict-prototypes']
>
> to native_client_sdk/src/libraries/nacl_io/library.dsc?


OK, done.  I assume CFLAGS does not get applied to C++ as well, otherwise
the compiler tends to warn that the flag doesn't apply to C++.

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[binji, 2014-06-07 00:05:14]

On 2014/06/06 23:52:13, Mark Seaborn wrote:
> On 6 June 2014 16:32, <mailto:binji@chromium.org> wrote:
> 
> > On 2014/06/06 23:12:39, Mark Seaborn wrote:
> >
> >> On 2014/06/06 23:00:14, Sam Clegg wrote:
> >> > Presumably we should be building nacl_io with this flag then, or at
> >> least
> >> > building that ninja target as part of out bot runs?
> >>
> >
> >  Ah, sorry, this is a dependency for this change:
> >> https://codereview.chromium.org/11415157/, which will just apply the flag
> >> globally for all untrusted C code.
> >>
> >
> >  It seemed easier to enable the flag globally (and fix the warnings) than
> >> to do
> >> it for NaCl-side code only, assuming I don't hit more instances of the
> >> warning.
> >
> >  It's more correct to use "(void)" anyway.
> >>
> >
> >  The Chromium trybot runs do include nacl_io -- chromium_builder_tests
> >> depends on
> >
> >  it.
> >>
> >
> > Yes, but we typically build nacl_io via the generated Makefiles, not GYP.
> > Can you add
> >
> > 'CFLAGS': ['-Wstrict-prototypes']
> >
> > to native_client_sdk/src/libraries/nacl_io/library.dsc?
> 
> 
> OK, done.  I assume CFLAGS does not get applied to C++ as well, otherwise
> the compiler tends to warn that the flag doesn't apply to C++.

I thought so, but it appears it doesn't. It looks like the correct place would
be to add it to
native_client_sdk/src/tools/common.mk:

NACL_CFLAGS ?= ...

But then it would apply to all .c files, which, combined with -Werror might
cause
some of the naclports builds to break. So maybe we should just omit this change
and fix it
later.

> 
> Cheers,
> Mark
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[binji, 2014-06-07 00:22:43]

lgtm

[Mark Seaborn, 2014-06-09 20:31:10]

On 6 June 2014 17:05, <binji@chromium.org> wrote:

> On 2014/06/06 23:52:13, Mark Seaborn wrote:
>
>  On 6 June 2014 16:32, <mailto:binji@chromium.org> wrote:
>>
>
>  > On 2014/06/06 23:12:39, Mark Seaborn wrote:
>> >
>> >> On 2014/06/06 23:00:14, Sam Clegg wrote:
>> >> > Presumably we should be building nacl_io with this flag then, or at
>> >> least
>> >> > building that ninja target as part of out bot runs?
>> >>
>> >
>> >  Ah, sorry, this is a dependency for this change:
>> >> https://codereview.chromium.org/11415157/, which will just apply the
>> flag
>> >> globally for all untrusted C code.
>> >>
>> >
>> >  It seemed easier to enable the flag globally (and fix the warnings)
>> than
>> >> to do
>> >> it for NaCl-side code only, assuming I don't hit more instances of the
>> >> warning.
>> >
>> >  It's more correct to use "(void)" anyway.
>> >>
>> >
>> >  The Chromium trybot runs do include nacl_io -- chromium_builder_tests
>> >> depends on
>> >
>> >  it.
>> >>
>> >
>> > Yes, but we typically build nacl_io via the generated Makefiles, not
>> GYP.
>> > Can you add
>> >
>> > 'CFLAGS': ['-Wstrict-prototypes']
>> >
>> > to native_client_sdk/src/libraries/nacl_io/library.dsc?
>>
>
>
>  OK, done.  I assume CFLAGS does not get applied to C++ as well, otherwise
>> the compiler tends to warn that the flag doesn't apply to C++.
>>
>
> I thought so, but it appears it doesn't. It looks like the correct place
> would be to add it to
> native_client_sdk/src/tools/common.mk:
>
> NACL_CFLAGS ?= ...
>
> But then it would apply to all .c files, which, combined with -Werror
> might cause
> some of the naclports builds to break. So maybe we should just omit this
> change and fix it
> later.


OK.  Changing CFLAGS did indeed break the build as a result of producing a
warning for C++ files, so I've reverted that change.  Was your "LGTM"
assuming that I keep or remove the CFLAGS change?

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[binji, 2014-06-09 20:57:04]

On 2014/06/09 20:31:10, Mark Seaborn wrote:
> On 6 June 2014 17:05, <mailto:binji@chromium.org> wrote:
> 
> > On 2014/06/06 23:52:13, Mark Seaborn wrote:
> >
> >  On 6 June 2014 16:32, <mailto:binji@chromium.org> wrote:
> >>
> >
> >  > On 2014/06/06 23:12:39, Mark Seaborn wrote:
> >> >
> >> >> On 2014/06/06 23:00:14, Sam Clegg wrote:
> >> >> > Presumably we should be building nacl_io with this flag then, or at
> >> >> least
> >> >> > building that ninja target as part of out bot runs?
> >> >>
> >> >
> >> >  Ah, sorry, this is a dependency for this change:
> >> >> https://codereview.chromium.org/11415157/, which will just apply the
> >> flag
> >> >> globally for all untrusted C code.
> >> >>
> >> >
> >> >  It seemed easier to enable the flag globally (and fix the warnings)
> >> than
> >> >> to do
> >> >> it for NaCl-side code only, assuming I don't hit more instances of the
> >> >> warning.
> >> >
> >> >  It's more correct to use "(void)" anyway.
> >> >>
> >> >
> >> >  The Chromium trybot runs do include nacl_io -- chromium_builder_tests
> >> >> depends on
> >> >
> >> >  it.
> >> >>
> >> >
> >> > Yes, but we typically build nacl_io via the generated Makefiles, not
> >> GYP.
> >> > Can you add
> >> >
> >> > 'CFLAGS': ['-Wstrict-prototypes']
> >> >
> >> > to native_client_sdk/src/libraries/nacl_io/library.dsc?
> >>
> >
> >
> >  OK, done.  I assume CFLAGS does not get applied to C++ as well, otherwise
> >> the compiler tends to warn that the flag doesn't apply to C++.
> >>
> >
> > I thought so, but it appears it doesn't. It looks like the correct place
> > would be to add it to
> > native_client_sdk/src/tools/common.mk:
> >
> > NACL_CFLAGS ?= ...
> >
> > But then it would apply to all .c files, which, combined with -Werror
> > might cause
> > some of the naclports builds to break. So maybe we should just omit this
> > change and fix it
> > later.
> 
> 
> OK.  Changing CFLAGS did indeed break the build as a result of producing a
> warning for C++ files, so I've reverted that change.  Was your "LGTM"
> assuming that I keep or remove the CFLAGS change?

I was assuming you'd remove it and we'd revisit later.

> 
> Cheers,
> Mark
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Mark Seaborn, 2014-06-09 21:09:03]

The CQ bit was checked by mseaborn@chromium.org

[commit-bot: I haz the power, 2014-06-09 21:11:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mseaborn@chromium.org/320103004/40001

[commit-bot: I haz the power, 2014-06-10 09:16:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[commit-bot: I haz the power, 2014-06-10 12:10:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-10 12:10:18]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[Mark Seaborn, 2014-06-10 14:14:19]

The CQ bit was checked by mseaborn@chromium.org

[commit-bot: I haz the power, 2014-06-10 14:16:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mseaborn@chromium.org/320103004/40001

[commit-bot: I haz the power, 2014-06-10 14:45:45]

Change committed as 276023