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

RFC: Fix unchecked accesses to Expected<T> #16275

Merged
merged 1 commit into from May 16, 2016

Conversation

4 participants
@MatthiasJReisinger
Contributor

MatthiasJReisinger commented May 9, 2016

Hi everyone,

when trying to build Julia together with LLVM 3.9, I noticed that the build fails with the following error message:

Expected<T> must be checked before access or destruction.
Expected<T> value was in success state. (Note: Expected<T> values in success mode must still be checked prior to being destroyed).
Aborted (core dumped)

My Make.user looks as follows:

USE_SYSTEM_LLVM:=1
LLVM_CONFIG:=$(MY_LLVM_BUILD)/bin/llvm-config

MY_LLVM_BUILD points to a manually built LLVM installation of version 3.9, retrieved from LLVM's git mirror and compiled via cmake && make (i.e. no options have been passed to the build). The error can also be reproduced with the following Make.user:

LLVM_VER:=svn
LLVM_ASSERTIONS:=1
LLVM_DEBUG:=1

There are a number of places in debuginfo.cpp and jitlayers.cpp where Expected<T> values are accessed without checking beforehand if they contain an Error. This results in the above error message and execution gets aborted.

The given pull request fixes these usages of Expected<T> such that the build process with LLVM 3.9 is working again.

Furthermore, after having fixed the build, it turned out that, when running make testall, a few tests also fail due to unchecked accesses to Expected<T> values. However, I've not yet been able to identify the particular test cases that fail, since the error messages do not appear consistently after the same test cases when running the tests repeatedly. Are the tests maybe running asynchronously such that their output does not necessarily appear in order?

Since this is my first pull request here, I apologize in case I overlooked anything important.

Any comments are appreciated, thank you!

@vtjnash

This comment has been minimized.

Member

vtjnash commented May 10, 2016

lgtm. you can run the test with JULIA_CPU_CORES=1 make test or equivalently make testall1 to get them to run linearly. otherwise it can be a bit of a guessing game to figure out which of the tests running in parallel caused the error. Otherwise, sometimes you can cross-reference between which test work died unexpectedly and figure out which test it was.

@MatthiasJReisinger MatthiasJReisinger force-pushed the MatthiasJReisinger:mjr/llvm_error_handling branch from 084e706 to efd4834 May 11, 2016

@MatthiasJReisinger MatthiasJReisinger force-pushed the MatthiasJReisinger:mjr/llvm_error_handling branch from efd4834 to 14d3c2b May 11, 2016

@MatthiasJReisinger

This comment has been minimized.

Contributor

MatthiasJReisinger commented May 11, 2016

Thank you! I've updated the PR, the tests are passing now.

@MatthiasJReisinger MatthiasJReisinger changed the title from WIP: Fix unchecked accesses to Expected<T> to RFC: Fix unchecked accesses to Expected<T> May 11, 2016

@vtjnash vtjnash merged commit f76d49c into JuliaLang:master May 16, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -343,7 +343,9 @@ class JuliaJITEventListener: public JITEventListener
for (const object::SymbolRef &sym_iter : debugObj.symbols()) {
StringRef sName;
#ifdef LLVM37
sName = sym_iter.getName().get();
auto sNameOrError = sym_iter.getName();
assert(sNameOrError);

This comment has been minimized.

@yuyichao

yuyichao May 16, 2016

Member

Is the issue that bool(sNameOrError) has to be called? It seems that this won't work for release build where assertion is disabled by default.

This comment has been minimized.

@MatthiasJReisinger

MatthiasJReisinger May 17, 2016

Contributor

Yes, the issue is that Expected<T>::operator bool() has to be called before invoking Expected<T>::get(), but only if LLVM was built with assertions enabled. So this would cause a problem only if you want to build Julia with assertions disabled together with LLVM where assertions are enabled, which I did not consider. But how can a release build of Julia be triggered? Because when I simply invoke the build process via make without any manual configuration, then Julia will still be built with assertions enabled. However, I think a release build of Julia would normally use a release build of LLVM anyway? Because then this should not cause any problems.

This comment has been minimized.

@yuyichao

yuyichao May 17, 2016

Member

I think assertion is disabled by default unless it is enabled by your CFLAGS.

This comment has been minimized.

@MatthiasJReisinger

MatthiasJReisinger May 17, 2016

Contributor

In src/Makefile the build rule for .c files uses the variable DISABLE_ASSERTIONS to pass
-DNDEBUG to the compiler by default, so indeed assertions will be enabled by default for .c files:

$(BUILDDIR)/%.o: $(SRCDIR)/%.c $(HEADERS) | $(BUILDDIR)
    @$(call PRINT_CC, $(CC) $(CPPFLAGS) $(CFLAGS) $(SHIPFLAGS) $(DISABLE_ASSERTIONS) -c $< -o $@)

However, the build rule for .cpp files does not use DISABLE_ASSERTIONS:

$(BUILDDIR)/%.o: $(SRCDIR)/%.cpp $(SRCDIR)/llvm-version.h $(HEADERS) $(LLVM_CONFIG_ABSOLUTE) | $(BUILDDIR)
    @$(call PRINT_CC, $(CXX) $(shell $(LLVM_CONFIG_HOST) --cxxflags) $(CPPFLAGS) $(CXXFLAGS) $(SHIPFLAGS) -c $< -o $@)

Hence, it seems that assertions will always be enabled when compiling the src/*.cpp files (unless -DNDEBUG will be passed manually). I'm wondering if this is intended?

This comment has been minimized.

@vtjnash

vtjnash May 17, 2016

Member

yes, for the CXX files, we inherit the NDEBUG flag from llvm-config

This comment has been minimized.

@MatthiasJReisinger

MatthiasJReisinger May 17, 2016

Contributor

You are right, thank you, I overlooked that. So there won't be issues with the release build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment