Fixes for gcc4.4

[ctiller]

No description provided.

[vjpai]

LGTM on changes ... wait to see if code works as expected.

[pgrosu]

Not sure this one works as expected. For example the following code below always returns true with or without the shadow warning parameter:

$ `gcc -std=c99 -Werror -o /dev/null -c shadow.c 2> /dev/null` && echo true || echo false
true
$ `gcc -std=c99 -Werror -Wshadow -o /dev/null -c shadow.c 2> /dev/null` && echo true || echo false
true

[vjpai]

The purpose of the check is to check to see if the compiler allows the shadow parameter in the expected way. The comparison would be to check the same command on gcc-4.4 vs gcc-4.8 or gcc-5.2

[pgrosu]

So just out of curiosity - and I'm sure you noticed something - but how might it behave differently among the versions? For example, in the manual for 5.3 you'll notice the following:

-Wshadow
Warn whenever a local variable or type declaration shadows another variable, parameter, type, class member (in C++), or instance variable (in Objective-C) or whenever a built-in function is shadowed. Note that in C++, the compiler warns if a local variable shadows an explicit typedef, but not if it shadows a struct/class/enum.

Usually if there was a difference among the versions, you would see something like this, specifying the version differences:

-Wpacked-bitfield-compat
The 4.1, 4.2 and 4.3 series of GCC ignore the packed attribute on bit-fields of type char. This has been fixed in GCC 4.4 but the change can lead to differences in the structure layout. GCC informs you when the offset of such a field has changed in GCC 4.4. For example there is no longer a 4-bit padding between field a and b in this structure:

          struct foo
          {
            char a:4;
            char b:8;
          } __attribute__ ((packed));

This warning is enabled by default. Use -Wno-packed-bitfield-compat to disable this warning.

Thanks,
~p

[pgrosu]

Sorry for naive question, but what was broken before that this PR is fixing? It looks like some of the tests are failing :(

[vjpai]

Some of the tests appear to be failing because of an issue with the Jenkins workers at the time of this build. If you dig into the console output, you'll see that the tests are passing but there is a problem in report generation. The team is getting this back in order soon.

[pgrosu]

Hi Vijay,

I looked at the console output and I still see failures. The reports seem to be generated fine.

If I go to the console output here:

https://grpc-testing.appspot.com/job/gRPC_pull_requests/6447/console

And then I go to one that shows failure gcov,c++,linux and go to this link for the HTML report:

https://grpc-testing.appspot.com/job/gRPC_pull_requests/config=gcov,language=c++,platform=linux/HTML_Report/

I believe you are using the following HTML publisher, but the plugin is not what's generating the failure:

https://github.com/jenkinsci/htmlpublisher-plugin

So let's dig on this one a little deeper. If I go to the interop console:

https://grpc-testing.appspot.com/job/gRPC_interop_pull_requests/2366/console

And look specifically at the following error:

[AR]      Creating /var/local/git/grpc/tmp/x86_64-linux/grpc_c/2.1.5/libs/opt/libz.a
src/core/support/env_linux.c: In function 'gpr_getenv':
src/core/support/env_linux.c:64:5: error: implicit declaration of function 'strstr' [-Werror=implicit-function-declaration]
     if (getenv_func != NULL && strstr(names[i], "secure") == NULL) {
     ^

This line point to line 64 of env_linux.c. This is usually due to not having #include <string.h> at the beginning of the file. On my system, you'll notice that in string.h the declaration is the following:

$ cat string.h | grep strstr
extern char *strstr (char *__haystack, __const char *__needle)
     __THROW __asm ("strstr") __attribute_pure__ __nonnull ((1, 2));
extern __const char *strstr (__const char *__haystack,
     __THROW __asm ("strstr") __attribute_pure__ __nonnull ((1, 2));
strstr (char *__haystack, __const char *__needle) __THROW
  return __builtin_strstr (__haystack, __needle);
strstr (__const char *__haystack, __const char *__needle) __THROW
  return __builtin_strstr (__haystack, __needle);
extern char *strstr (__const char *__haystack, __const char *__needle)
/* Similar to `strstr' but this function ignores the case of both strings.  */
$

But you have #include "src/core/support/string.h" on line 53. So now let's look inside src/core/support/string.h, which is located here:

https://github.com/ctiller/grpc/blob/3163a46f37eeb34ce950fdabda06c100ffd13f07/src/core/support/string.h

But if I look inside it there is not declaration of strstr.

By any chance do you have the design specifications for gRPC that details the functionality and operations of the intended source code? With that it will make it straightforward to address all these issues.

Thank you,
Paul

[vjpai]

Thanks for looking a little deeper. Yes, I was only looking at a few of them where the reports were getting stuck. The remaining failures will certainly need to be taken care of before merging. But in answer to your original question, the purpose of the PR is to resolve failures related to using this with gcc 4.4, which was triggering shadow warnings on the use of "index" and "close" in boringssl.

[jtattermusch]

With our setup, ADD is actually a bit dangerous. We currently name the docker image after building with a sha1 of the Dockerfile itself. So if you happen to update the post-git-setup.sh script later, that change will get picked up (I think), but concurrent runs of older and newer versions of our code will keep rewriting the dockerimage to use (and you'll never be sure which version you are actually running).

[jtattermusch]

I reviewed the Docker and run_tests related part of the PR (see inline comments). Leaving review of makefile and C core changes to @vjpai (I did a quick pass I and I was basically happy about that part).

[ctiller]

@jtattermusch - Updated the docker stuff, PTAL

[jtattermusch]

LGTM, did it pass with gcc4.4 locally?

[ctiller]

Yeah. Just pushed a commit to fix the strstr problem when
-DGPR_BACKWARDS_COMPATIBILITY_MODE is used.

On Wed, Feb 24, 2016 at 3:49 PM Jan Tattermusch [email protected]
wrote:

LGTM, did it pass with gcc4.4 locally?

—
Reply to this email directly or view it on GitHub
#5384 (comment).

[pgrosu]

Thanks Craig :)

[pgrosu]

Hi Vijay,

It should be fairly simple to update boringssl to not trigger shadow warnings via #pragma GCC diagnostic ignored "-Wshadow". Below is an example:

$ cat shadow.c
int main(void) {

        int i;

        { int i; }

}
$ gcc -Wshadow shadow.c
shadow.c: In function âmainâ:
shadow.c:5:8: warning: declaration of âiâ shadows a previous local [-Wshadow]
  { int i; }
        ^
shadow.c:3:6: warning: shadowed declaration is here [-Wshadow]
  int i;
      ^
$ cat shadow-ignore.c
int main(void) {

#pragma GCC diagnostic ignored "-Wshadow"
        int i;

#pragma GCC diagnostic ignored "-Wshadow"
        { int i; }

}
$ gcc -Wshadow shadow-ignore.c
$

Hope it helps,
~p

[jtattermusch]

@vjpai is this fine to merge?