Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

WIP: src: make build pass with GCC < 4.5 #9098

Closed
wants to merge 1 commit into from

Conversation

misterdjules
Copy link

Building node with GCC > 4.4 on CentOS makes the node binary depend on a
more recent version of the C/C++ runtime that is not installed by
default on these older CentOS platforms, and probably on other platforms
as well.

Building node with the default gcc and g++ compilers that come with
these older versions of CentOS allows to ship a node binary that runs
out of the box on these setups with older C/C++ runtimes.

This change works around a bug that was fixed in GCC 4.5. Versions of
GCC < 4.5 would not support using the injected-class-name of a
template base class as a type name.

This change also fixes a lot of strict-aliasing warnings due to type
casts in the src/queue.h headers.

Fixes #9079.

@misterdjules misterdjules changed the title src: make build pass with GCC < 4.5 WIP: src: make build pass with GCC < 4.5 Jan 26, 2015
@misterdjules misterdjules added this to the 0.11.16 milestone Jan 26, 2015
@misterdjules
Copy link
Author

This is a first draft, as there are different ways to solve this problem and I'm looking to gather your feedback.

The fix for the "stric-aliasing" warnings in src/queue.h is more or less a revert of joyent/libuv@0520464 that was cherry-picked in joyent/node with d29fe0f.
I'm not familiar with the inappropriate uses of the QUEUE API mentioned in the original commit message, so I would be happy if @bnoordhuis could give us some context.

Another way to fix the warnings would of course be to pass the -fno-strict-aliasing option to gcc and g++, similar to what had been been done in the past or to what is done currently on OSX for node.

This change also doesn't take care of the queue implementation in libuv, but it would also need to be fixed.

Regarding the changes that work around GCC's bug with injected class name of a template base class as a type name, it makes the code less maintainable as it duplicates the template parameter. There are various ways to mitigate that:

  1. Use a typedef to write the template parameter once and use this new type instead at both places of the fully qualified template name twice. This way, if the template parameter needs to change, it only needs to be updated in one place.

  2. Get rid of most of the additional sub-classes of ReqWrap that were created but which implementation are empty (like GetAddrInfoReqWrap) and instead directly create instances of ReqWrap. For sub-classes of ReqWrap that don't have an empty implementation, use solution 1.

@tjfontaine @trevnorris @bnoordhuis @chrisdickinson @cjihrig I'm looking forward to reading your comments.

@trevnorris
Copy link

The fundamentals of the fix LGTM. Though @bnoordhuis would be the best person for this review.

@bnoordhuis
Copy link
Member

I'm not familiar with the inappropriate uses of the QUEUE API mentioned in the original commit message, so I would be happy if @bnoordhuis could give us some context.

The original is essentially untyped. The macros evaluate to a void* pointer, which in C is implicitly coercible to almost any other pointer type. It was too easy to use in a wrong way, so I made it a little stronger typed.

If you want to fix up queue.h, you can do better than this. Compare QUEUE_DATA() with ContainerOf(): the former is an untyped C macro, the latter is a strongly typed C++ template function. You can do a similar thing for the QUEUE data structure.

For background, I copied queue.h from libuv because I wanted a quick tailq implementation. It stuck because most people working on the C++ bits were familiar with it.

@misterdjules
Copy link
Author

Thank you @bnoordhuis for the info!

I took another approach with misterdjules@a541b92.

Basically, making my changes to queue.h type safe would require more thoughts and more time than what I have before the release of v0.11.16, and I don't want to land them if they make the queue implementation less safe.

Instead, I propose to pass -fno-strict-aliasing to gcc and g++ when gcc version is <= 4.4 as a temporary solution. This change would also require floating a patch for libuv. Later, we would make the queue implementation compile without -fno-strict-aliasing while being as safe as the current implementation.

I ran benchmarks (bench-http, bench-fs and bench-tls, not bench-net as it was hanging and I wanted to get some results quickly) on a CentOS 6.6 VM with 4GB of RAM and 4 CPUs. It seems that the binary compiled with -fno-strict-aliasing doesn't have any major performance regression compared to the one without.

The benchmarks' output is not easy to compare with separate files. I just saw how to run benchmarks and output compared results with benchmark/compare.js, I'll post the output here as soon as possible.

@misterdjules
Copy link
Author

The output of benchmark/compare.js is now available as a gist.

@tjfontaine
Copy link

this LGTM, thanks!

Building node with GCC > 4.4 on CentOS makes the node binary depend on a
more recent version of the C/C++ runtime that is not installed by
default on these older CentOS platforms, and probably on other platforms
as well.

Building node with the default gcc and g++ compilers that come with
these older versions of CentOS allows to ship a node binary that runs
out of the box on these setups with older C/C++ runtimes.

This change works around a bug that was fixed in GCC 4.5. Versions of
GCC < 4.5 would not support using the injected-class-name of a
template base class as a type name.

This change also disables aliasing optimizations for toolchains using
GCC <= 4.4.

Fixes nodejs#9079.
misterdjules pushed a commit to misterdjules/node that referenced this pull request Jan 30, 2015
Building node with GCC > 4.4 on CentOS makes the node binary depend on a
more recent version of the C/C++ runtime that is not installed by
default on these older CentOS platforms, and probably on other platforms
as well.

Building node with the default gcc and g++ compilers that come with
these older versions of CentOS allows to ship a node binary that runs
out of the box on these setups with older C/C++ runtimes.

This change works around a bug that was fixed in GCC 4.5. Versions of
GCC < 4.5 would not support using the injected-class-name of a
template base class as a type name.

This change also disables aliasing optimizations for toolchains using
GCC <= 4.4 as they're not able to deal with the aliasing in the queue
implementation used by libuv and node (see src/queue.h).

Fixes nodejs#9079.

PR: nodejs#9098
PR-URL: nodejs#9098
Reviewed-By: Timothy J Fontaine <tjfontaine@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@misterdjules
Copy link
Author

Landed in 5926526, tests pass on all platforms, and more specifically on the CentOS 5.7 agent without loading a more recent libstdc++.

Thank you!

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

Successfully merging this pull request may close these issues.

None yet

6 participants