This is an archive of the discontinued LLVM Phabricator instance.

[lldb/PlatformPOSIX] Change LoadImage default to RTLD_LAZY
ClosedPublic

Authored by vsk on Mar 18 2021, 11:13 AM.

Details

Summary

In general, it seems like the debugger should allow programs to load & run with
libraries as far as possible, instead of defaulting to being super-picky about
unavailable symbols.

This is critical on macOS/Darwin, as libswiftCore.dylib may 1) export a version
symbol using @available markup and then 2) expect that other exported APIs are
only dynamically used once the version symbol is checked. We can't open a
version of the library built with a bleeding-edge SDK on an older OS without
RTLD_LAXY (or pervasive/expensive @available markup added to dyld APIs).

See: https://lists.llvm.org/pipermail/lldb-dev/2021-March/016796.html

Diff Detail

Event Timeline

vsk created this revision.Mar 18 2021, 11:13 AM
vsk requested review of this revision.Mar 18 2021, 11:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2021, 11:13 AM
vsk planned changes to this revision.Mar 18 2021, 11:15 AM

This needs a test.

shafik added a subscriber: shafik.Mar 18 2021, 1:44 PM

Thank you for adding the commentary on what RTLD_LAZY

vsk added a comment.EditedMar 18 2021, 2:02 PM

The best way I can think of to test this is to:

  • Make a library, T1, out of:
% cat t1.c
extern void use();
void f1() {}
void f2() { use(); }
  • Make a library, T2, out of:
% cat t2.c
void use() {}
  • Link T1.dylib against T2.dylib, so that the reference to use isn't undefined.
  • Now, rebuild T2.dylib using an empty source file (delete the definition of use).
  • Launch a process, then do "process load T1.dylib"; this will only succeed under RTLD_LAZY.

Any concerns?

This and the proposed test sound reasonable to me.

lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
581

I don't think the libswiftCore.dylib example adds much.

vsk updated this revision to Diff 331694.Mar 18 2021, 2:47 PM
  • Add a test. It's Darwin-specific as I couldn't work out how to pass the .dylib path to the linker in a platform-agnostic way.
  • Trim the comment.
vsk updated this revision to Diff 331695.Mar 18 2021, 2:53 PM
  • Make the test generic (not Darwin-specific)
kastiglione added inline comments.Mar 19 2021, 1:17 PM
lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
613

Instead of DLOPEN_OPTIONS and string concatenation, what do you think about defining RTLD_LAZY inside this string of code?

static const int RTLD_LAZY = 0x1;

/// code…

result_ptr->image_ptr = dlopen(name, RTLD_LAZY);

The main reason I suggest this is putting the name inline like that makes it more grep friendly.

vsk updated this revision to Diff 331998.Mar 19 2021, 2:06 PM
  • Define RTLD_LAZY in the expression as suggested
vsk updated this revision to Diff 331999.Mar 19 2021, 2:08 PM
  • Delete an unused '#undef DLOPEN_OPTIONS'
This revision is now accepted and ready to land.Mar 19 2021, 2:09 PM
This revision was landed with ongoing or failed builds.Mar 19 2021, 3:14 PM
This revision was automatically updated to reflect the committed changes.
Harbormaster completed remote builds in B94788: Diff 331999.
vsk updated this revision to Diff 332024.Mar 19 2021, 4:05 PM

Add asserts checking that the library paths exist, and restrict the test to Darwin. See https://bugs.llvm.org/show_bug.cgi?id=49656

This revision was landed with ongoing or failed builds.Mar 19 2021, 4:06 PM

I think you should remove: lldb/test/API/.lit_test_times.txt

vsk added a comment.Mar 22 2021, 9:05 AM

Sorry about that. The file was removed in 8248dd91d7f042893d4a605e98d19cb1b89a44d4.