Hello,
Patch 1 squashes these fixes into a single diff
- r124942 removed a sys/wait.h inclusion. This is incorrect for Linux.
- SetArch was removed which breaks the build on Linux.
- Linux doesn't support RTLD_FIRST so apply that mode conditionally.
Patch 2 fixes a missing include.
On an unrelated note, is there a git mirror of lldb? Would patches
created with git be okay?
lldb_host_cpp.patch (1.47 KB)
lldb_RegisterContextLinux_x86_64.patch (771 Bytes)
Jai ++
Thanks for rectifying these linux problems. These patches look just fine from my standpoint … though i can’t commit them myself.
Since i’m the one who contributed to these problems for linux, i have in my queue of things to set up: a linux build on ubuntu to make certain you don’t have to fix these things again in the future.
Regards,
++ Kirk
lldb_host_cpp.patch (1.52 KB)
ATT00001.htm (225 Bytes)
lldb_RegisterContextLinux_x86_64.patch (792 Bytes)
ATT00001.htm (1.42 KB)
Jai Menon <jmenon86@gmail.com> writes:
Hello,
Patch 1 squashes these fixes into a single diff
- r124942 removed a sys/wait.h inclusion. This is incorrect for Linux.
Looks good.
- SetArch was removed which breaks the build on Linux.
Looks OK to unbreak the build. Eventually we should probably return an
ArchSpec based on the host compile time triple (say from
llvm/Config/config.h).
- Linux doesn't support RTLD_FIRST so apply that mode conditionally.
Why conditionally? Perhaps we should just not pass this flag at all so
that we get similar behavior from this method on all posixy systems --
just a thought.
Patch 2 fixes a missing include.
Looks good.
On an unrelated note, is there a git mirror of lldb? Would patches
created with git be okay?
I create all my patches using git and send them to lldb-commits for
review. There is no official lldb git mirror. I try to keep a git tree
up to date (time permitting) here: GitHub - eightcien/lldb: Linux work for the LLDB debugger
Linux work is on the lldb-linux branch.
Jai Menon <jmenon86@gmail.com> writes:
Hello,
Patch 1 squashes these fixes into a single diff
- r124942 removed a sys/wait.h inclusion. This is incorrect for Linux.Looks good.
- SetArch was removed which breaks the build on Linux.
Looks OK to unbreak the build. Eventually we should probably return an
ArchSpec based on the host compile time triple (say from
llvm/Config/config.h).
I am in the process of adding a patch that adds more members to lldb_private::ArchSpec:
lldb::ByteOrder m_byte_order;
uint32_t m_addr_byte_size;
lldb::Triple m_triple;
So now the ArchSpec class will have:
class ArchSpec {
...
protected:
lldb::ArchitectureType m_type;
uint32_t m_cpu;
uint32_t m_sub;
lldb::ByteOrder m_byte_order;
uint32_t m_addr_byte_size;
lldb::Triple m_triple;
};
This will allows for architectures to be specified in an alternate byte order (big endian ARM) and also allow us to water down the current specific arch down to a target triple that has more generic enums for the arch, vendor and OS.
Many classes that have static FindPlugin() functions current take an ArchSpec which, in its current form, isn't enough to make an informed decision on which plug-in to chose. With the lldb::Triple added, it will be much easier and more correct. This patch touches a bunch of areas of the code, but I should commit it soon.
- Linux doesn't support RTLD_FIRST so apply that mode conditionally.
Why conditionally? Perhaps we should just not pass this flag at all so
that we get similar behavior from this method on all posixy systems --
just a thought.
I will check to make sure RTLD_FIRST isn't something we need on MacOSX with our dynamic linker guru, and if not, then I will leave the code as is. Else, I will move a copy of Host::DynamicLibraryOpen() over into Host.mm (the apple specific Host functions) and conditionally compile it out in Host.cpp for non-apple variants.
This will allows for architectures to be specified in an alternate byte order (big endian ARM) and also allow us to water down the current specific arch down to a target triple that has more generic enums for the arch, vendor and OS.
Many classes that have static FindPlugin() functions current take an ArchSpec which, in its current form, isn't enough to make an informed decision on which plug-in to chose. With the lldb::Triple added, it will be much easier and more correct. This patch touches a bunch of areas of the code, but I should commit it soon.
- Linux doesn't support RTLD_FIRST so apply that mode conditionally.
Why conditionally? Perhaps we should just not pass this flag at all so
that we get similar behavior from this method on all posixy systems --
just a thought.I will check to make sure RTLD_FIRST isn't something we need on MacOSX with our dynamic linker guru, and if not, then I will leave the code as is. Else, I will move a copy of Host::DynamicLibraryOpen() over into Host.mm (the apple specific Host functions) and conditionally compile it out in Host.cpp for non-apple variants.
So looking at our darwin man pages for dlopen, the RTLD_FIRST seems like a nice flag to specify:
RTLD_FIRST The retuned handle is tagged so that any dlsym() calls on the handle will only search the image specified, and not subsequent images. If path is NULL and the option RTLD_FIRST is used, the handle returned will only search the main executable.
We don't use NULL for the file handle for calls to dlopen, though we could. Is there an equivalent option on dlsym() on linux? I don't want to do code like:
FileSpec plugin_spec ("/plugins/disassembler.dylib");
void *lib_handle = Host::DynamicLibraryOpen (plugin_spec, error);
if (lib_handle)
{
void *init_callback = Host::DynamicLibraryGetSymbol (lib_handle, "InitPlugin", error);
if (init_callback)
{
// Now on linux we might have a function that comes from another shared library...
}
}
Due to the global symbol namespace we will run into issue unless we open things locally (RTLD_LOCAL), this is how unix dl calls only looks into a single shared library. The main problem is the RTLD_GLOBAL is sticky, so if anyone has opened a dynamic library already it will already be global and it will be subject to dlsym() calls that kick up any symbol in any shared library...
A few solutions to this is to define our own DynamicLibrary struct that contains both a "void *" for the handle, and a FileSpec for the path to the shared library. Then any calls to Host::DynamicLibraryGetSymbol will need to verify their symbols came from the shared library mentioned in the "void *".
I will submit a patch that should fix these issues as well as add some extra args to the Host::DynamicLibraryOpen and Host::DynamicLibraryGetSymbol calls that will allow for more control over this process.
Greg Clayton
Greg Clayton <gclayton@apple.com> writes:
So looking at our darwin man pages for dlopen, the RTLD_FIRST seems
like a nice flag to specify:RTLD_FIRST The retuned handle is tagged so that any dlsym() calls on
the handle will only search the image specified, and not subsequent
images. If path is NULL and the option RTLD_FIRST is used, the handle
returned will only search the main executable.We don't use NULL for the file handle for calls to dlopen, though we
could. Is there an equivalent option on dlsym() on linux? I don't want
to do code like:
No equivalent option that I know of.
FileSpec plugin_spec ("/plugins/disassembler.dylib");
void *lib_handle = Host::DynamicLibraryOpen (plugin_spec, error);
if (lib_handle)
{
void *init_callback = Host::DynamicLibraryGetSymbol (lib_handle, "InitPlugin", error);
if (init_callback)
{
// Now on linux we might have a function that comes from another shared library...
}
}
OK. I think we should document DynamicLibaryOpen and friends so that
the behavior is understood. I often think of "resolving a symbol in a
shared library" as a process which includes lookups wrt object
dependencies since a "logical" library interface may very well be
implemented using multiple shared objects (a c++ wrapper around a c
library for example).
Due to the global symbol namespace we will run into issue unless we
open things locally (RTLD_LOCAL), this is how unix dl calls only looks
into a single shared library. The main problem is the RTLD_GLOBAL is
sticky, so if anyone has opened a dynamic library already it will
already be global and it will be subject to dlsym() calls that kick up
any symbol in any shared library...A few solutions to this is to define our own DynamicLibrary struct
that contains both a "void *" for the handle, and a FileSpec for the
path to the shared library. Then any calls to
Host::DynamicLibraryGetSymbol will need to verify their symbols came
from the shared library mentioned in the "void *".
This is exactly what we will need on linux: DynamicLibraryGetSymbol
will need to call dladdr(3) (glibc-specific) and compare the resolved vs
expected library paths.
I will submit a patch that should fix these issues as well as add some
extra args to the Host::DynamicLibraryOpen and
Host::DynamicLibraryGetSymbol calls that will allow for more control
over this process.
Great. I'll watch for the commits and implement/test anything that is
needed on the linux side.
With revision 125064 I have modified the way Host::DynamicLibaryOpen works. It now takes an abtract options that can be logical OR'ed together that allow one to specify lazy, local/global, and if Host::DynamicLibraryGetSymbol lookups should only find matches in the shared library that was opened. Systems that support RTLD_FIRST will use it, and ones that don't will do a bit of extra work to make sure this open request (find only symbols in this shared library) will be honored.
Stephen, please try it out on Linux and see if this works as you would expect.
I also switched over to either defining LLDB_CONFIG_XXX values, or not defining them to stay inline with current open source practices so we can soon use autoconf or cmake to configure LLDB.
Greg Clayton
Greg Clayton <gclayton@apple.com> writes:
With revision 125064 I have modified the way Host::DynamicLibaryOpen
works. It now takes an abtract options that can be logical OR'ed
together that allow one to specify lazy, local/global, and if
Host::DynamicLibraryGetSymbol lookups should only find matches in the
shared library that was opened. Systems that support RTLD_FIRST will
use it, and ones that don't will do a bit of extra work to make sure
this open request (find only symbols in this shared library) will be
honored.Stephen, please try it out on Linux and see if this works as you would
expect.
Tested with the patch below. I think ensuring that one of RTLD_LAZY or
RTLD_NOW is set should be correct for darwin as well.
I also switched over to either defining LLDB_CONFIG_XXX values, or not
defining them to stay inline with current open source practices so we
can soon use autoconf or cmake to configure LLDB.
Nice! Thanks a lot for doing that!
Greg Clayton
diff --git a/source/Host/common/Host.cpp b/source/Host/common/Host.cpp
index b017461..6c157ef 100644
--- a/source/Host/common/Host.cpp
+++ b/source/Host/common/Host.cpp
@@ -673,6 +673,8 @@ Host::DynamicLibraryOpen (const FileSpec &file_spec, uint32_t options, Error &er
if (options & eDynamicLibraryOpenOptionLazy)
mode |= RTLD_LAZY;
+ else
+ mode |= RTLD_NOW;
if (options & eDynamicLibraryOpenOptionLocal)
mode |= RTLD_LOCAL;
@@ -744,7 +746,7 @@ Host::DynamicLibraryGetSymbol (void *opaque, const char *symbol_name, Error &err
// This host doesn't support limiting searches to this shared library
// so we need to verify that the match came from this shared library
// if it was requested in the Host::DynamicLibraryOpen() function.
- if (dylib_info->options & eDynamicLibraryOpenOptionLimitGetSymbol)
+ if (dylib_info->open_options & eDynamicLibraryOpenOptionLimitGetSymbol)
{
FileSpec match_dylib_spec (Host::GetModuleFileSpecForHostAddress (symbol_addr));
if (match_dylib_spec != dylib_info->file_spec)
Agreed and nice catches:
% svn commit
Sending source/Host/common/Host.cpp
Transmitting file data .
Committed revision 125084.