Page MenuHomePhabricator

Add support for deterministically linked binaries on macOS to lldb.
ClosedPublic

Authored by erikchen on Aug 6 2019, 1:40 PM.

Details

Summary

When ld64 links a binary deterministically using the flag ZERO_AR_DATE, it sets
a timestamp of 0 for N_OSO members in the symtab section, rather than the usual
last modified date of the object file. Prior to this patch, lldb would compare
the timestamp from the N_OSO member against the last modified date of the object
file, and skip loading the object file if there was a mismatch. This patch
updates the logic to ignore the timestamp check if the N_OSO member has
timestamp 0.

The original logic was added in https://reviews.llvm.org/rL181631 as a safety
check to avoid problems when debugging if the object file was out of date. This
was prior to the introduction of deterministic build in ld64. lld still doesn't
support deterministic build.

Other code in llvm already relies on and uses the assumption that a timestamp of
0 means deterministic build. For example, commit
9ccfddc39d4d27f9b16fcc72ab30d483151d6d08 adds similar timestamp checking logic
to dsymutil, but special cases timestamp 0. Likewise, commit
0d1bb79a0413f221432a7b1d0d2d10c84c4bbb99 adds a long comment describing
deterministic archive, which mostly uses timestamp 0 for determinism.

Diff Detail

Event Timeline

erikchen created this revision.Aug 6 2019, 1:40 PM
Herald added a project: Restricted Project. · View Herald Transcript
erikchen retitled this revision from Add support for deterministically linked binaries to lldb. to Add support for deterministically linked binaries on macOS to lldb..Aug 6 2019, 1:41 PM
erikchen edited the summary of this revision. (Show Details)
erikchen edited the summary of this revision. (Show Details)
rnk added a comment.Aug 6 2019, 1:48 PM

Code seems fine, I think this is probably the right fix, but this probably deserves a test along the lines of what's in the commit message. I'm not familiar with LLDB's test suite, so I can't give much guidance there.

Let's see if @JDevlieghere has input.

JDevlieghere requested changes to this revision.Aug 6 2019, 2:02 PM

I agree with Reid, change looks fine but this definitely needs a test.

The Python-style tests in packages/Python/lldbsuite/test give you the most freedom as they use makefiles to build inferiors, but a lit test might be easier and less boilerplate.

This revision now requires changes to proceed.Aug 6 2019, 2:02 PM
erikchen updated this revision to Diff 213758.Aug 6 2019, 5:24 PM

Added a test.

Okay, this is ready for another round of review when you have time. Thanks :)

JDevlieghere added inline comments.Aug 6 2019, 5:41 PM
lldb/lit/SymbolFile/DWARF/deterministic-build.cpp
1 ↗(On Diff #213758)

You'll want a REQUIRES: system-darwin here.

6 ↗(On Diff #213758)

If you check for stop reason = breakpoint instead of the source line, you don't have to work around the comments getting printed. You wouldn't even need an input file anymore either.

// RUN: %lldb %t -o "breakpoint set -name main" -o "run"  -o "exit" | FileCheck %s
// CHECK: stop reason = breakpoint
erikchen updated this revision to Diff 213960.Aug 7 2019, 11:50 AM
erikchen marked an inline comment as done.
erikchen marked an inline comment as done.
erikchen added inline comments.
lldb/lit/SymbolFile/DWARF/deterministic-build.cpp
6 ↗(On Diff #213758)

I'm sorry but I don't understand how that would work. Even if object file symbols aren't loaded, lldb is still able to set a breakpoint at main, e.g.:

/Users/erikchen/projects/llvm-project/build/bin/lldb --no-lldbinit -S /Users/erikchen/projects/llvm-project/build/tools/lldb/lit/lit-lldb-init /Users/erikchen/projects/llvm-project/build/tools/lldb/lit/SymbolFile/DWARF/Output/deterministic-build.cpp.tmp -o "breakpoint set -name main" -o "run" -o "exit"
(lldb) command source -s 0 '/Users/erikchen/projects/llvm-project/build/tools/lldb/lit/lit-lldb-init'
Executing commands in '/Users/erikchen/projects/llvm-project/build/tools/lldb/lit/lit-lldb-init'.
(lldb) # LLDB init file for the LIT tests.
(lldb) settings set symbols.enable-external-lookup false
(lldb) settings set plugin.process.gdb-remote.packet-timeout 60
(lldb) settings set interpreter.echo-comment-commands false
(lldb) target create "/Users/erikchen/projects/llvm-project/build/tools/lldb/lit/SymbolFile/DWARF/Output/deterministic-build.cpp.tmp"
Current executable set to '/Users/erikchen/projects/llvm-project/build/tools/lldb/lit/SymbolFile/DWARF/Output/deterministic-build.cpp.tmp' (x86_64).
(lldb) breakpoint set -name main
error: deterministic-build.cpp.tmp debug map object file '/Users/erikchen/projects/llvm-project/build/tools/lldb/lit/SymbolFile/DWARF/Output/deterministic-build.cpp.tmp.o' has changed (actual time is 2019-08-07 11:40:55.000000000, debug map time is 1969-12-31 16:00:00.000000000) since this executable was linked, file will be ignored
Breakpoint 1: where = deterministic-build.cpp.tmp`main, address = 0x0000000100000fa0
(lldb) run
Process 18131 stopped

  • thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1 frame #0: 0x0000000100000fa0 deterministic-build.cpp.tmp`main

deterministic-build.cpp.tmp`main:
-> 0x100000fa0 <+0>: pushq %rbp

0x100000fa1 <+1>: movq   %rsp, %rbp
0x100000fa4 <+4>: xorl   %eax, %eax
0x100000fa6 <+6>: movl   $0x0, -0x4(%rbp)

Process 18131 launched: '/Users/erikchen/projects/llvm-project/build/tools/lldb/lit/SymbolFile/DWARF/Output/deterministic-build.cpp.tmp' (x86_64)
(lldb) exit

labath added a subscriber: labath.Aug 7 2019, 11:59 AM
labath added inline comments.
lldb/lit/SymbolFile/DWARF/deterministic-build.cpp
6 ↗(On Diff #213758)

Maybe you could set a file+line breakpoint instead of function one. That would only work if line tables get parsed (which live in the .o file if I know my MachO correctly). That way you wouldn't even have to run the process, which means this test might one day work on other systems too (if/when lld is able to link MachO files reasonably).

erikchen updated this revision to Diff 213962.Aug 7 2019, 12:07 PM
erikchen marked 3 inline comments as done.
erikchen added inline comments.
lldb/lit/SymbolFile/DWARF/deterministic-build.cpp
6 ↗(On Diff #213758)

that worked, thanks. I've updated the patch.

JDevlieghere accepted this revision.Aug 7 2019, 12:11 PM

Thanks, this LGTM!

This revision is now accepted and ready to land.Aug 7 2019, 12:11 PM
erikchen marked an inline comment as done.Aug 7 2019, 12:17 PM

I do not have commit access. Can someone land the change for me? Thanks.

erikchen edited the summary of this revision. (Show Details)Aug 7 2019, 12:24 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2019, 12:29 PM