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 C++ implementation C++11 only. #2780

Closed
liujisi opened this issue Mar 1, 2017 · 48 comments
Closed

Make C++ implementation C++11 only. #2780

liujisi opened this issue Mar 1, 2017 · 48 comments
Assignees
Labels

Comments

@liujisi
Copy link
Contributor

liujisi commented Mar 1, 2017

C++11 has been out for several years and we are thinking of moving to allow C++11 features from being used in the code base. That means the implementation will not be able to compile if you don't have a C++11 compiler.

We may create a branch that works for C++98. The branch will only accept bug fixes, but not new features, optimization, etc

Please reply in this thread if you think this would be an issue for your project.

@liujisi liujisi added the c++ label Mar 1, 2017
@koubaa
Copy link

koubaa commented Mar 2, 2017

Hello,

Thank you for collecting feedback about this. My project consists of shared libraries on linux. Some libraries are built with c++11 and others must be built with c++98 (for compatibility with some other libraries). We link protobuf dynamically so this may not an issue for us, unless any c++11 language features will be used in header files that our c++98 libraries use.

@matthauck
Copy link
Contributor

This would be an issue for our project. We support back to centos5 (gcc 4.1) and a number of platforms in between for which we are not yet able to turn on the c++11 flag. One of the attractive things about protobuf was its impressive backwards support, and would be sad to have older platforms be stuck with less.

@matthauck
Copy link
Contributor

(PS. I feel bad saying this since I would love to move to c++11 only for our project too, haha! It is a sad world that we can't use "new" features from 6 years go yet. I'm sure libraries feel this pain more than applications do even. All of which to say, I feel your pain! 😭 )

@vjpai
Copy link
Contributor

vjpai commented Mar 10, 2017

Hi there, we made a related decision in gRPC - version 1.0 supports older compilers with only limited C++11 support (not as few as protobuf, though), but version 1.1 and beyond only support true C++11 compilers (as of grpc/grpc#8602 ). I understand, though, that protobuf's predicament is greater since it is a package that has been public for much longer, has more users, and has been supporting C++98.

@gerben-s
Copy link
Contributor

I strongly favor moving to a 4.x C++11 only branch. Users tight to pre-c++11 can continue using the 3.x branch which we support in maintenance mode.

Note that linking pre-c++11 and post-c++11 libraries is already very tricky due to the standard libraries being slightly incompatible (see https://gcc.gnu.org/wiki/Cxx11AbiCompatibility). Hence I think the impact of not being able to link libs depending on pre-c++11 and post-c++11 proto runtime is minor, given you already have dangerous issues lurking around.

Furthermore, for c++ protos, we already have the rule that all proto files in a binary have to be compiled with the exact same version of the compiler and runtime.

@os12
Copy link

os12 commented Mar 10, 2017

We have C++14 enabled at work so this change would be welcome. Protobuf has great compatibility w.r.t. the serialized format, so people who are stuck with old environments can continue using the old libs.

@daomuzhiwang
Copy link

daomuzhiwang commented Mar 13, 2017

Hello all,

Now I'm using gcc-5.4.0 to compile protobuf. However it couldn't be built because of C++11 new features.
I tried to modify "Makefile" to add compile options "-std=c++11" into "CXXFLAGS", but it still didn't work. How could I make it?

Solution:

I used command "configure CXXFLAGS=-std=c++11" to config protobuf.

@ghost
Copy link

ghost commented Mar 13, 2017

+1 , backporting gcc5.4.x /7.x on old unmaintained distros is straightforward also with proper stripping unneeded language target at configure build flag.

@jakirkham
Copy link

Requiring C++11 will make it basically impossible to support protobuf on some Python versions on Windows. In particular, Python 2.7 support will not be possible and Python 3.5 will need to be a minimum requirement. This is assuming that protobuf continues to need to have compiled portions for these Python versions. The reason being that CPython ties each major minor version of the interpreter to a particular Visual Studio version on Windows. As a result, any extension one builds needs to be built with the same runtime as the interpreter or risk segfaults. A full listing can be found in this reference. Note that Python 2.6 and 2.7 requires Visual Studio 2008 (VC 9), which does not have C++11 support. Also Python 3.4 requires Visual Studio 2010 (VC 10), which also does not have C++11 support. Only Python 3.5 and 3.6, which requires Visual Studio 2015 (VC 14), have C++11 support.

cc @xfxyjwf

@os12
Copy link

os12 commented Mar 13, 2017

With respect to @jakirkham's note, Python does not let people build their recent recent releases with older Visual Studio versions. IMO Protobuf should follow suit. There will be no way to move forward otherwise.

Also note that Visual Studio 2010 has basic C++11 support, see the official reference.

@Armagetron
Copy link

The googlei18n/libphonenumber project which uses protobuf currently doesn't support C++11. See google/libphonenumber#1594 for the discussion.

@lumpidu
Copy link

lumpidu commented May 1, 2017

Make it a 4.x and require C++11. Should be a clean slate. C++98 is old now and people should really make up their minds using a newer C++ version. There are so many benefits, it's not even funny.

jsonn pushed a commit to jsonn/pkgsrc that referenced this issue May 2, 2017
2017-04-05 version 3.3.0 (C++/Java/Python/PHP/Objective-C/C#/Ruby/JavaScript)
  Planned Future Changes
  * There are some changes that are not included in this release but are
    planned for the near future:
      - Preserve unknown fields in proto3: please read this doc:

          https://docs.google.com/document/d/1KMRX-G91Aa-Y2FkEaHeeviLRRNblgIahbsk4wA14gRk/view

        for the timeline and follow up this github issue:

          protocolbuffers/protobuf#272

        for discussion.
      - Make C++ implementation C++11 only: we plan to require C++11 to build
        protobuf code starting from 3.4.0 or 3.5.0 release. Please join this
        github issue:

          protocolbuffers/protobuf#2780

        to provide your feedback.

  C++
  * Fixed map fields serialization of DynamicMessage to correctly serialize
    both key and value regardless of their presence.
  * Parser now rejects field number 0 correctly.
  * New API Message::SpaceUsedLong() that’s equivalent to
    Message::SpaceUsed() but returns the value in size_t.
  * JSON support
    - New flag always_print_enums_as_ints in JsonPrintOptions.
    - New flag preserve_proto_field_names in JsonPrintOptions. It will instruct
      the JSON printer to use the original field name declared in the .proto
      file instead of converting them to lowerCamelCase when printing JSON.
    - JsonPrintOptions.always_print_primtive_fields now works for oneof message
      fields.
    - Fixed a bug that doesn’t allow different fields to set the same json_name
      value.
    - Fixed a performance bug that causes excessive memory copy when printing
      large messages.
  * Various performance optimizations.

  Java
  * Map field setters eagerly validate inputs and throw NullPointerExceptions
    as appropriate.
  * Added ByteBuffer overloads to the generated parsing methods and the Parser
    interface.
  * proto3 enum's getNumber() method now throws on UNRECOGNIZED values.
  * Output of JsonFormat is now locale independent.

  Python
  * Added FindServiceByName() in the pure-Python DescriptorPool. This works only
    for descriptors added with DescriptorPool.Add(). Generated descriptor_pool
    does not support this yet.
  * Added a descriptor_pool parameter for parsing Any in text_format.Parse().
  * descriptor_pool.FindFileContainingSymbol() now is able to find nested
    extensions.
  * Extending empty [] to repeated field now sets parent message presence.

  PHP
  * Added file option php_class_prefix. The prefix will be prepended to all
    generated classes defined in the file.
  * When encoding, negative int32 values are sign-extended to int64.
  * Repeated/Map field setter accepts a regular PHP array. Type checking is
    done on the array elements.
  * encode/decode are renamed to serializeToString/mergeFromString.
  * Added mergeFrom, clear method on Message.
  * Fixed a bug that oneof accessor didn’t return the field name that is
    actually set.
  * C extension now works with php7.
  * This is the first GA release of PHP. We guarantee that old generated code
    can always work with new runtime and new generated code.

  Objective-C
  * Fixed help for GPBTimestamp for dates before the epoch that contain
    fractional seconds.
  * Added GPBMessageDropUnknownFieldsRecursively() to remove unknowns from a
    message and any sub messages.
  * Addressed a threading race in extension registration/lookup.
  * Increased the max message parsing depth to 100 to match the other languages.
  * Removed some use of dispatch_once in favor of atomic compare/set since it
    needs to be heap based.
  * Fixes for new Xcode 8.3 warnings.

  C#
  * Fixed MapField.Values.CopyTo, which would throw an exception unnecessarily
    if provided exactly the right size of array to copy to.
  * Fixed enum JSON formatting when multiple names mapped to the same numeric
    value.
  * Added JSON formatting option to format enums as integers.
  * Modified RepeatedField<T> to implement IReadOnlyList<T>.
  * Introduced the start of custom option handling; it's not as pleasant as it
    might be, but the information is at least present. We expect to extend code
    generation to improve this in the future.
  * Introduced ByteString.FromStream and ByteString.FromStreamAsync to
    efficiently create a ByteString from a stream.
  * Added whole-message deprecation, which decorates the class with [Obsolete].

  Ruby
  * Fixed Message#to_h for messages with map fields.
  * Fixed memcpy() in binary gems to work for old glibc, without breaking the
    build for non-glibc libc’s like musl.

  Javascript
  * Added compatibility tests for version 3.0.0.
  * Added conformance tests.
  * Fixed serialization of extensions: we need to emit a value even if it is
    falsy (like the number 0).
  * Use closurebuilder.py in favor of calcdeps.py for compiling JavaScript.
@hzhxxx
Copy link

hzhxxx commented May 3, 2017

support,use c++ 11 later

@rhadesan
Copy link

Hi,

we use WindRiver compiler, and there is no support for C++11 there. Unfortunately it is not possible to switch to different compiler for us (company defined compiler...).

If you switch, do you plan to support older releases?

@liujisi
Copy link
Contributor Author

liujisi commented May 10, 2017

If we go with C++11 only, we will probably maintain a branch and only back port severe bug fixes.

@ibroheem
Copy link

Why don't you just fast forward to C++14?
Lots of goodies!

@TACIXAT
Copy link

TACIXAT commented Jul 26, 2017

There are a lot of projects that aren't C++11 compatible.

@arthur-tacca
Copy link

I notice that move constructors for generated messages has landed in master. Fantastic! But there still aren't move constructors/assignments for the containers RepeatedField, RepeatedPtrFieldBase, RepeatedPtrField<T>. (I mean for the whole object, rather than for the elements they contain.) Any chance of adding these too? I think it would be useful and a very small change.

(I hope it's OK that I cross posted this from #2791; this seems to be the main report.)

@zzm3145
Copy link

zzm3145 commented Sep 26, 2017

According to the plan, 3.5 will start to reserve unknown fields. I hope 3.5 will be C++98 compatible, so old compilers will get this important feature in their final release.

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Sep 26, 2017

@zzm3145 the next 3.5.0 release will not require C++11. We will likely start to require C++11 when we migrate to use Abseil: Google's C++ common libraries:
https://opensource.googleblog.com/2017/09/introducing-abseil-new-common-libraries.html

@kkm000
Copy link

kkm000 commented Nov 16, 2017

I do not know how efficient are the calls guarded by GoogleInitOnce() (they happen once per message parameterless constructor invocation), but replacing them with C++11 singleton guarantee on the generated fileset's TableStruct would certainly remove the efficiency concerns away from the platform (trusting the platform to do that is the best way, in my opinion).

As implemented, unless doing link-time code generation, it seems to me that GoogleInitOnce() is a real call into the library, with a real call frame setup and prefetch pipeline kaput at the very least.

@gerben-s
Copy link
Contributor

@kkm000 With C++11 support removing GoogleOnceInit is indeed something we want to do.

However GoogleOnceInit is implemented smart enough that after inlining the function it's a atomic read on a global variable and only a call in the initialization case. However using c++11 support will remove this dependency and allow the compiler to do whatever is best for the platform and is more readable.

@kkm000
Copy link

kkm000 commented Nov 17, 2017

@gerben-s: Yup, my bad, I misread the inline piece. It only calls GoogleOnceInitImpl (which is out-of-line) only after the inline lock-free flag test. D-oh!

Yes, c++11 and its library removes a lot of low-level stuff, like atomics, mutexes, threads, futures, you name it. Great stuff! atomicops_internals_x86_msvc.h? Oh, not any more! :)

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Dec 18, 2017

An update from protobuf team: it's decided starting from version 3.6.0 protobuf will require C++11 to compile. A 3.5.x branch will be maintained for pre-C++11 compilers but only bug fixes will be accepted in the branch. We are already accepting C++11 features into our internal code base and will accept pull requests using C++11 features on the github repo as well.

@jakirkham
Copy link

FWIW had seen some C++11 features in 3.5.0 as well. So it would be good to tidy that up if it is intended to work on pre-C++11 compilers. See these comments for details. Haven't checked recently to see if they have been fixed.

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Dec 18, 2017

@jakirkham I checked your build log but it's using v3.4.1. Have you tried 3.5.0? The issue you pointed to doesn't exist in 3.5.0 as far as I can tell.

@jakirkham
Copy link

Had backported some patches from 3.5.0 to 3.4.1, but I agree that is not the same as building 3.5.0. Have retried fresh with 3.5.0 and ran into issue ( #4064 ).

@jakirkham
Copy link

Ran into an issue with 3.5.1 that we missed. Raised as issue ( #4094 ).

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Jan 16, 2018
2017-12-20 version 3.5.1 (C++/Java/Python/PHP/Objective-C/C#/Ruby/JavaScript)
  Planned Future Changes
  * Make C++ implementation C++11 only: we plan to require C++11 to build
    protobuf code starting from 3.6.0 release. Please join this github issue:
    protocolbuffers/protobuf#2780 to provide your feedback.

  protoc
  * Fixed a bug introduced in 3.5.0 and protoc in Windows now accepts non-ascii
    characters in paths again.

  C++
  * Removed several usages of C++11 features in the code base.
  * Fixed some compiler warnings.

  PHP
  * Fixed memory leak in C-extension implementation.
  * Added discardUnknokwnFields API.
  * Removed duplicatd typedef in C-extension headers.
  * Avoided calling private php methods (timelib_update_ts).
  * Fixed Any.php to use fully-qualified name for DescriptorPool.

  Ruby
  * Added Google_Protobuf_discard_unknown for discarding unknown fields in
    messages.

  C#
  * Unknown fields are now preserved by default.
  * Floating point values are now bitwise compared, affecting message equality
    check and Contains() API in map and repeated fields.
@jart
Copy link
Contributor

jart commented Apr 27, 2018

@jayantkolhe @pherl Is it possible to build portable C++11 Linux release binaries on any system that isn't RHEL6? Here's some knowledge from my notes file:

  • RHEL 5 (2007) went EOL 2017-03 (cc: @matthauck). It used kernel2.6.18, glibc2.5 and gcc4.1 which supported -std=c++98 (GPLv2 w/ runtime exception)
  • RHEL 6 (2011) doesn't go EOL until 2020-11. It uses kernel2.6.32, libc2.12, GCC 4.4, supports -std=c++0x flag (GPLv3 w/ GCC RUNTIME LIBRARY EXCEPTION v3.1). If you build C++ binaries on RHEL6 they should work on pretty much any Linux since 2011.
  • Debian 8 (2015) doesn't go EOL until 2020-05. It uses libc2.19, gcc4.9, has complete -std=c++11 support. If you build binaries on Debian 8, they should work on pretty much any Linux since 2014.
  • Ubuntu 16+ uses GCC 5+. After much work, the TensorFlow team determined it's impossible to use these newer versions of GCC to create dynamically linked release binaries that will actually run on any Linux distro before 2016. This can be verified by checking ldd -v for CXXABI_1.3.8.

I'd like to know if it's possible to use something like musl-cross-make to statically link musl-libc (MIT) and LLVM libc++ (MIT). In that case, we'd be able to write C++14 code, compile it with GCC6+ (which can do things like -ftree-vectorize AVX-512) and have our release binaries work on Linux 2.6+ (~2006).

@jakirkham
Copy link

At conda-forge, we are migrating to some new compilers produced by Anaconda (they already use them) that are based on GCC 7.2 that @mingwandroid worked on. We would still use them to build on CentOS 6, but as glibc is generally backwards compatible, the binaries built should still work on newer Linux systems. Maybe this is interesting to you?

@jart
Copy link
Contributor

jart commented Apr 27, 2018 via email

@mingwandroid
Copy link

No, we provide our own libstdc++. You could link to it statically I guess (we don't though).

@mingwandroid
Copy link

The toolchain is a CentOS6 glibc 2.12 targeting (and hosted) pseudo-cross compiler. It is used to build all of the Anaconda Distribution on Linux.

@jart
Copy link
Contributor

jart commented Apr 27, 2018

That's one solution but protobuf can't assume an Anaconda subsystem is available when distributing binaries on PyPi. PEP 513 says manylinux1 binaries must work on CentOS 5, which can be ignored, but ideally shouldn't.

Take for example gcc-7.2.0-i486-linux-musl.tar.xz which is 31mB; runs on RHEL4+ i486+; can turn helloworld.c into a 5kb static binary that runs on any Linux and can be optimized for any microarchitecture, e.g. Skylake. It also supports C++17. The catch is GPLv3 if you want portable binaries.

@mingwandroid
Copy link

Yup, and thanks for paying attention to the manylinux1 specs. manylinux2010 targets RHEL6/glibc 2.12 though so it may be possible to consider our tools (with static libstdc++) for that?

I like that musl toolchain; very cool.

@jart
Copy link
Contributor

jart commented Apr 28, 2018

I heard from @gunan that manylinux2010 doesn't work in practice, i.e. manylinux1 has to be used anyway, even if it can't run on RHEL5, because certain tools will break. I'm not recommending statically linking libstdc++, which could raise legal questions. Also note GNU changed the license of libstdc++ when C++11 support was introduced. That's why libc++ exists, but I don't know how to use it.

@mingwandroid
Copy link

Any more details you can provide about this manylinux2010 issue would be welcome.

@gunan
Copy link

gunan commented Apr 30, 2018 via email

@yifeif
Copy link

yifeif commented May 9, 2018

I think manylinux2010 should be okay? But it might not be fully rolled out yet pypa/manylinux#179

@acozzette
Copy link
Member

I'm going to close this issue because this is now pretty much done and we're using C++11 now. Feel free to comment if you have any C++11-related issues, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests