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

build: always specify PORTABLE=1 when building RocksDB #58997

Merged

Conversation

petermattis
Copy link
Collaborator

Something recently changed on the TC agents (presumably usage of a new
machine type with a different CPU) so that using -march=native when
building RocksDB causes AVX512 instructions to be used which are not
present on roachprod machines. The result is that binaries built in this
way will die with "illegal instruction". RocksDB has a mechanism to
disable the use of -march=native: pass PORTABLE=1 to the make
command. We were already doing this for release builds. Now we're doing
it for all builds.

Fixes #58754

Release note: none

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@petermattis
Copy link
Collaborator Author

Note that I only suspect that this will fix the problem as I'm not able to reproduce it locally. It is also possible that we'll need to remove the use of -march=native from cryptopp. I poked around and didn't see any obvious way to do so.

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

thanks

@knz
Copy link
Contributor

knz commented Jan 14, 2021

we'll want this on all the release branches, since we're still releasing fixes as far as 19.2.

@petermattis
Copy link
Collaborator Author

I think I can kick off a roachtest run on a specific branch. I'll try doing so and see what happens.

@petermattis
Copy link
Collaborator Author

we'll want this on all the release branches, since we're still releasing fixes as far as 19.2.

Ack. Assuming this works I'll patch 19.2 and 20.2 as well.

@petermattis
Copy link
Collaborator Author

https://teamcity.cockroachdb.com/viewQueued.html?itemId=2583885 is the roachtest run on this PR.

@@ -638,7 +638,7 @@ $(ROCKSDB_DIR)/Makefile: $(C_DEPS_DIR)/rocksdb-rebuild | bin/.submodules-initial
@# NOTE: If you change the CMake flags below, bump the version in
@# $(C_DEPS_DIR)/rocksdb-rebuild. See above for rationale.
cd $(ROCKSDB_DIR) && CFLAGS+=" $(sse)" && CXXFLAGS+=" $(sse)" && cmake $(xcmake-flags) $(ROCKSDB_SRC_DIR) \
$(if $(findstring release,$(BUILDTYPE)),-DPORTABLE=ON) -DWITH_GFLAGS=OFF \
-DPORTABLE=ON -DWITH_GFLAGS=OFF \
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment explaining what this will do. I assume it will prevent use of march=native?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this prevents use of -march=native. Comment added.

Something recently changed on the TC agents (presumably usage of a new
machine type with a different CPU) so that using `-march=native` when
building RocksDB causes AVX512 instructions to be used which are not
present on roachprod machines. The result is that binaries built in this
way will die with "illegal instruction". RocksDB has a mechanism to
disable the use of `-march=native`: pass `PORTABLE=1` to the make
command. We were already doing this for release builds. Now we're doing
it for all builds.

Fixes cockroachdb#58754

Release note: none
@RaduBerinde
Copy link
Member

That build failed (perhaps because of pushing a new commit?). Started a new one here: https://teamcity.cockroachdb.com/viewLog.html?buildId=2584351

@RaduBerinde
Copy link
Member

This one failed too, with a very strange bin/roachtest: No such file or directory.

@petermattis
Copy link
Collaborator Author

make: *** No rule to make target 'vendor/modules.txt', needed by 'bin/.bootstrap'.  Stop.
make: *** Waiting for unfinished jobs....

That is from the make bin/roachtest ... invocation. Very strange as the 20.1 branch uses dep.

@petermattis
Copy link
Collaborator Author

This is probably the problem. From the Roachtest Nightly - GCE build step:

if [[ "${TC_BUILD_BRANCH}" == "release-20.1" ]] || [[ "${TC_BUILD_BRANCH}" == "release-20.2" ]]; then
    CLOUD=${TARGET_CLOUD} BUILD_TAG="${build_tag}" ./build/teamcity-nightly-roachtest.sh
    exit
fi

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

Successfully merging this pull request may close these issues.

4 participants