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

Jump to bottom

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

Merged
merged 1 commit into from May 16, 2016
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/codegen.cpp
View file
Open in desktop
Expand Up @@ -1273,14 +1273,18 @@ static uint64_t compute_obj_symsize(const object::ObjectFile *obj, uint64_t offs
uint64_t Addr;
object::section_iterator Sect = ESection;
#ifdef LLVM38
Sect = Sym.getSection().get();
auto SectOrError = Sym.getSection();
assert(SectOrError);
Sect = SectOrError.get();
#else
if (Sym.getSection(Sect)) continue;
#endif
if (Sect == ESection) continue;
if (Sect != Section) continue;
#ifdef LLVM37
Addr = Sym.getAddress().get();
auto AddrOrError = Sym.getAddress();
assert(AddrOrError);
Addr = AddrOrError.get();
#else
if (Sym.getAddress(Addr)) continue;
#endif
Expand Down
32 changes: 24 additions & 8 deletions src/debuginfo.cpp
View file
Open in desktop
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

sName = sNameOrError.get();
#else
sym_iter.getName(sName);
#endif
Expand All @@ -357,14 +359,20 @@ class JuliaJITEventListener: public JITEventListener
if (pAddr) {
uint64_t Addr, SectionAddr, SectionLoadAddr;
#if defined(LLVM38)
Addr = sym_iter.getAddress().get();
Section = sym_iter.getSection().get();
auto AddrOrError = sym_iter.getAddress();
assert(AddrOrError);
Addr = AddrOrError.get();
auto SectionOrError = sym_iter.getSection();
assert(SectionOrError);
Section = SectionOrError.get();
assert(Section != EndSection && Section->isText());
SectionAddr = Section->getAddress();
Section->getName(sName);
SectionLoadAddr = getLoadAddress(sName);
#elif defined(LLVM37)
Addr = sym_iter.getAddress().get();
auto AddrOrError = sym_iter.getAddress();
assert(AddrOrError);
Addr = AddrOrError.get();
sym_iter.getSection(Section);
assert(Section != EndSection && Section->isText());
Section->getName(sName);
Expand Down Expand Up @@ -419,14 +427,20 @@ class JuliaJITEventListener: public JITEventListener
for(const auto &sym_size : symbols) {
const object::SymbolRef &sym_iter = sym_size.first;
#ifdef LLVM39
object::SymbolRef::Type SymbolType = sym_iter.getType().get();
auto SymbolTypeOrError = sym_iter.getType();
assert(SymbolTypeOrError);
object::SymbolRef::Type SymbolType = SymbolTypeOrError.get();
#else
object::SymbolRef::Type SymbolType = sym_iter.getType();
#endif
if (SymbolType != object::SymbolRef::ST_Function) continue;
uint64_t Addr = sym_iter.getAddress().get();
auto AddrOrError = sym_iter.getAddress();
assert(AddrOrError);
uint64_t Addr = AddrOrError.get();
#ifdef LLVM38
Section = sym_iter.getSection().get();
auto SectionOrError = sym_iter.getSection();
assert(SectionOrError);
Section = SectionOrError.get();
#else
sym_iter.getSection(Section);
#endif
Expand All @@ -441,7 +455,9 @@ class JuliaJITEventListener: public JITEventListener
uint64_t SectionLoadAddr = L.getSectionLoadAddress(secName);
#endif
Addr -= SectionAddr - SectionLoadAddr;
StringRef sName = sym_iter.getName().get();
auto sNameOrError = sym_iter.getName();
assert(sNameOrError);
StringRef sName = sNameOrError.get();
uint64_t SectionSize = Section->getSize();
size_t Size = sym_size.second;
#if defined(_OS_WINDOWS_)
Expand Down
8 changes: 5 additions & 3 deletions src/jitlayers.cpp
View file
Open in desktop
Expand Up @@ -269,13 +269,15 @@ class JuliaOJIT {
continue;
if (!(Flags & object::BasicSymbolRef::SF_Exported))
continue;
auto Name = Symbol.getName();
orc::JITSymbol Sym = JIT.CompileLayer.findSymbolIn(H, *Name, true);
auto NameOrError = Symbol.getName();
assert(NameOrError);
auto Name = NameOrError.get();
orc::JITSymbol Sym = JIT.CompileLayer.findSymbolIn(H, Name, true);
assert(Sym);
// note: calling getAddress here eagerly finalizes H
// as an alternative, we could store the JITSymbol instead
// (which would present a lazy-initializer functor interface instead)
JIT.LocalSymbolTable[*Name] = (void*)(uintptr_t)Sym.getAddress();
JIT.LocalSymbolTable[Name] = (void*)(uintptr_t)Sym.getAddress();
}
}
}
Expand Down