[llvm-jitlink] Allow optional stub-kind filter in stub_addr() expressions

[weliveindetail]

We use jitlink-check lines in LIT tests as the primary tool for testing JITLink backends. Parsing and evaluation of the expressions is implemented in RuntimeDyldChecker. The stub_addr(obj, name) expression allows to obtain the linker-generated stub for the external symbol name in object file obj. This is limiting JITLink backends to a single stub for an external symbol.

This patch adds support for an index parameter: stub_addr(obj, name, index=0). It allows us to inspect more than a single stub in our tests and, respectively, it enables JITLink backends to emit multiple stubs. This is necessary for the AArch32 backend, which must be able to emit two different kinds of stubs depending on the instruction set state (Arm/Thumb) of the relocation site. Since the new parameter is optional, we don't have to update existing tests.

[weliveindetail]

I made a dedicated review for the first commit in #78365. Once it's through I will rebase this one and the NFC commit will vanish.

[weliveindetail]

A test that actually exercises the index != 0 case will follow in #78371

[lhames]

What about using a generic string key rather than an index? So uses would look like:

stub_addr("foo.o", func, plt-stub)
stub_addr("foo.o", func, thumb-stub)
...

We don't need the test infrastructure to be fast, and this might make it easier to read?

[weliveindetail]

That's a great idea! I changed the stub index into a filter expression, so that we can match kinds flexibly like this:

stub_addr("foo.o", func, thumb)
stub_addr("foo.o", func, thumbv7)
stub_addr("foo.o", func, armv7_abs_le)
stub_addr("foo.o", func, .*v7_.*_le)

Actual detection could be implemented like this: 142168f#diff-9dbc5aacbecdc751e46355a2f0416a448e5eaea70e9ee6c82916be34b1381fd9L1269

[lhames]

Very cool!

LGTM. Thanks @weliveindetail!

[mgorny]

This change broke building on i686 (possibly all 32-bit arches) with GCC (possibly with clang too):

[15/16] Building CXX object tools/llvm-jitlink/CMakeFiles/llvm-jitlink.dir/llvm-jitlink.cpp.o
FAILED: tools/llvm-jitlink/CMakeFiles/llvm-jitlink.dir/llvm-jitlink.cpp.o 
CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/bin/c++ -DGTEST_HAS_RTTI=0 -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -D_LARGEFILE_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/mgorny/git/llvm-project/build/tools/llvm-jitlink -I/home/mgorny/git/llvm-project/llvm/tools/llvm-jitlink -I/home/mgorny/git/llvm-project/build/include -I/home/mgorny/git/llvm-project/llvm/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -m32 -Werror=date-time -fno-lifetime-dse -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-nonnull -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Os -DNDEBUG -std=c++17  -fno-exceptions -funwind-tables -fno-rtti -MD -MT tools/llvm-jitlink/CMakeFiles/llvm-jitlink.dir/llvm-jitlink.cpp.o -MF tools/llvm-jitlink/CMakeFiles/llvm-jitlink.dir/llvm-jitlink.cpp.o.d -o tools/llvm-jitlink/CMakeFiles/llvm-jitlink.dir/llvm-jitlink.cpp.o -c /home/mgorny/git/llvm-project/llvm/tools/llvm-jitlink/llvm-jitlink.cpp
/home/mgorny/git/llvm-project/llvm/tools/llvm-jitlink/llvm-jitlink.cpp: In member function ‘llvm::Error llvm::Session::FileInfo::registerStubEntry(LinkGraph&, Symbol&, GetSymbolTargetFunction)’:
/home/mgorny/git/llvm-project/llvm/tools/llvm-jitlink/llvm-jitlink.cpp:1215:65: error: invalid initialization of reference of type ‘llvm::SmallVector<llvm::RuntimeDyldChecker::MemoryRegionInfo>&’ from expression of type ‘llvm::SmallVector<llvm::RuntimeDyldChecker::MemoryRegionInfo, 1>’
 1215 |   SmallVector<MemoryRegionInfo> &Entry = StubInfos[TS->getName()];
      |                                                                 ^
/home/mgorny/git/llvm-project/llvm/tools/llvm-jitlink/llvm-jitlink.cpp: In member function ‘llvm::Error llvm::Session::FileInfo::registerMultiStubEntry(LinkGraph&, Symbol&, GetSymbolTargetFunction)’:
/home/mgorny/git/llvm-project/llvm/tools/llvm-jitlink/llvm-jitlink.cpp:1233:69: error: invalid initialization of reference of type ‘llvm::SmallVector<llvm::RuntimeDyldChecker::MemoryRegionInfo>&’ from expression of type ‘llvm::SmallVector<llvm::RuntimeDyldChecker::MemoryRegionInfo, 1>’
 1233 |   SmallVector<MemoryRegionInfo> &Entry = StubInfos[Target->getName()];
      |                                                                     ^
ninja: build stopped: subcommand failed.

[weliveindetail]

This change broke building on i686 (possibly all 32-bit arches) with GCC (possibly with clang too)

Thanks for reporting! I cross-compiled it on ubuntu20.04 with GCC-10 for arm-linux-gnueabihf, which is 32-bit and it doesn't reproduce the issue. I will have a closer look tomorrow.

Is there a public-facing bot that reproduces it?

[mgorny]

I don't think so.

I've used the following CMake invocation to reproduce it in source tree:

cmake <monorepo>/llvm -G Ninja -DLLVM_TARGETS_TO_BUILD=X86 -DCMAKE_BUILD_TYPE=MinSizeRel -DLLVM_CCACHE_BUILD=ON -DLLVM_BUILD_32_BITS=ON

[nikic]

I haven't verified, but I believe a43c192 should fix this. (Generally, when taking references to SmallVector, you almost always want to use SmallVectorImpl instead.)

[weliveindetail]

Thanks! Yes, I was thinking about something like that as well. @mgorny Does it fix the issue for you?

[mgorny]

Thanks. I can confirm that it builds now.