Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make BoringSSL symbols private to gRPC in Obj-C so there is no conflict when linking with OpenSSL. #16358

Merged
merged 2 commits into from
Aug 24, 2018

Conversation

muxi
Copy link
Member

@muxi muxi commented Aug 15, 2018

The change is to add prefix to BoringSSL symbols in gRPC Objective-C to avoid conflicting with OpenSSL for apps that depend on both SSL libraries (see detailed discussions in #6111). This PR makes gRPC to prefix its own BoringSSL symbols with GRPC_SHADOW_ to avoid conflict with OpenSSL.

@muxi muxi added lang/ObjC release notes: yes Indicates if PR needs to be in release notes labels Aug 15, 2018
@grpc-testing
Copy link

[trickle] No significant performance differences

@nicolasnoble
Copy link
Member

How is this shadow file being generated? How do we make sure it's up to date when we update the dependency?

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@muxi
Copy link
Member Author

muxi commented Aug 15, 2018

Yep I have script that needs to be polished then uploaded. Current commits are just to verify if tests run ok.

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

[trickle] No significant performance differences

1 similar comment
@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
   724,591  BoringSSL (>)        722,941

 1,954,384      Total (>)      1,952,734

***************FRAMEWORKS****************
  New size                      Old size
 3,617,842       Core (>)      3,616,490
 3,416,723  BoringSSL (>)      3,326,168

10,755,494      Total (>)     10,663,570


@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

1 similar comment
@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
   724,591  BoringSSL (>)        722,941

 1,954,384      Total (>)      1,952,734

***************FRAMEWORKS****************
  New size                      Old size
 3,617,842       Core (>)      3,616,490
 3,416,723  BoringSSL (>)      3,326,168

10,755,489      Total (>)     10,663,576


@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
   724,186  BoringSSL (>)        722,941

 1,953,979      Total (>)      1,952,734

***************FRAMEWORKS****************
  New size                      Old size
 3,617,842       Core (>)      3,616,490
 3,391,467  BoringSSL (>)      3,326,168

10,730,226      Total (>)     10,663,576


@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
   724,186  BoringSSL (>)        722,941

 1,953,963      Total (>)      1,952,718

***************FRAMEWORKS****************
  New size                      Old size
 3,617,842       Core (>)      3,616,490
 3,391,467  BoringSSL (>)      3,326,168

10,730,228      Total (>)     10,663,585


@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

BUILD Outdated
@@ -1543,6 +1543,7 @@ grpc_cc_library(
"grpc_base",
"grpc_transport_chttp2_alpn",
"tsi",
"shadow_boringssl",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason to pick this name "shadow_boringssl"? How about a more intuitive name like "boringssl_grpc" since you are using BoringSSL-Grpc in the podsec.

BUILD Outdated
grpc_cc_library(
name = "shadow_boringssl",
hdrs = [
"src/core/tsi/boringssl_shadow.h",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use consistent naming. Rename boringssl_shadow.h to shadow_boringssl.h. boringssl_grpc.h sounds more intuitive.

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
   724,802  BoringSSL (>)        722,941

 1,954,445      Total (>)      1,952,584

***************FRAMEWORKS****************
  New size                      Old size
 3,638,246       Core (>)      3,618,638
 3,483,131  BoringSSL (>)      3,326,168

10,842,300      Total (>)     10,665,722


@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None]      +8  +0.0%

  [ = ]       0 TOTAL       +8  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
   724,802  BoringSSL (>)        722,941

 1,954,445      Total (>)      1,952,584

***************FRAMEWORKS****************
  New size                      Old size
 3,638,230       Core (>)      3,618,618
 3,483,131  BoringSSL (>)      3,326,168

10,842,279      Total (>)     10,665,713


@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@@ -0,0 +1,32 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: check_boringssl_commit.sh is not a great name. You are checking if the "shadow boringssl symbol list is up to date", the fact that you are looking whether the commit sha in a file is the same is implementation detail. IMHO, seeing "commit" in the script name doesn't tell me anything useful

I suggest renaming to check_boringssl_symbol_list.sh, check_boringssl_shadow_symbol_list.sh or sth in that sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check_shadow_boringssl_symbol_list.sh is perhaps the best name, but leaving up to you.

Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last comment about the check_boringssl_commit.sh and after that I'm happy with the change.

LGTM and thanks!

@muxi muxi force-pushed the privatize-boringssl branch from 4e90cc6 to b24b212 Compare August 23, 2018 16:43
@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None]      +8  +0.0%

  [ = ]       0 TOTAL       +8  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
   724,802  BoringSSL (>)        722,941

 1,954,445      Total (>)      1,952,584

***************FRAMEWORKS****************
  New size                      Old size
 3,638,230       Core (>)      3,618,618
 3,483,131  BoringSSL (>)      3,326,168

10,842,281      Total (>)     10,665,704


@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None]      +8  +0.0%

  [ = ]       0 TOTAL       +8  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@muxi
Copy link
Member Author

muxi commented Aug 24, 2018

Bazel TSAN build for C/C++ - #15827
Bazel Debug build for C/C++ - #15827
iOS Binary Size Diff (internal CI) - build system flake

@muxi muxi merged commit 35479b8 into grpc:master Aug 24, 2018
@muxi muxi deleted the privatize-boringssl branch August 24, 2018 16:35
@muxi muxi added release notes: no Indicates if PR should not be in release notes and removed release notes: yes Indicates if PR needs to be in release notes labels Sep 10, 2018
@srini100 srini100 changed the title Make symbols of BoringSSL private Make BoringSSL symbols private to gRPC in Obj-C so there is no conflict when linking with OpenSSL. Sep 10, 2018
@muxi muxi added release notes: yes Indicates if PR needs to be in release notes and removed release notes: no Indicates if PR should not be in release notes labels Sep 10, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Dec 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang/ObjC priority/P0/RELEASE BLOCKER release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants