This is an archive of the discontinued LLVM Phabricator instance.

[lldb][x86_64] Add fs_base/gs_base support for Linux
ClosedPublic

Authored by yinghuitan on Jul 13 2023, 7:56 PM.

Details

Summary

This patch adds fs_base/gs_base support for Linux x86_64.

GDB supports fs_base/gs_base registers while LLDB does not. Since both linux coredump note section and ptrace
supports them it is a missing feature.

For context, this is a required feature to support getting pthread pointer on linux from both live and dump debugging.
See thread below for details:
https://discourse.llvm.org/t/how-to-get-pthread-pointer-from-lldb/70542/2?u=jeffreytan81

Implementation wise, we have initially tried #ifdef approach to reuse the code but it is introducing very tricky bugs and proves
hard to maintain. Instead the diff completely separates the registers between x86_64 and x86_64_with_base so that non-linux related
implementations can use x86_64 registers while linux uses x86_64_with_base.
Here are the list of changes done in the patch:

  • Registers in lldb-x86-register-enums.h are separated into two: x86_64 and x86_64_with_base
  • fs_base/gs_base are added into x86_64_with_base
  • All registers are renamed from lldb_xxx_x86_64 => x86_64::lldb_xxx across code base
  • All macros are changed to use x86_64::lldb_xxx as well
  • All linux files are change to use x86_64::lldb_xxx => x86_64_with_base::lldb_xxx
  • NativeRegisterContextDBReg_x86.cpp
  • Support linux elf-core:
    • A new RegisterContextLinuxCore_x86_64 class is added for ThreadElfCore
    • RegisterContextLinuxCore_x86_64 overrides and uses its own register set supports fs_base/gs_base
    • RegisterInfos_x86_64_with_base/RegisterInfos_x86_64_with_base_shared ared added to provide g_contained_XXX/g_invalidate_XXX and RegInfo related code sharing.
  • RegisterContextPOSIX_x86 ::m_gpr_x86_64 seems to be unused so I removed it.
  • NativeRegisterContextDBReg_x86::GetDR() is overridden in NativeRegisterContextLinux_x86_64 to make watchpoint work.

Diff Detail

Event Timeline

yinghuitan created this revision.Jul 13 2023, 7:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2023, 7:56 PM
yinghuitan requested review of this revision.Jul 13 2023, 7:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2023, 7:56 PM
yinghuitan edited the summary of this revision. (Show Details)Jul 13 2023, 8:05 PM
yinghuitan edited the summary of this revision. (Show Details)Jul 13 2023, 8:10 PM
yinghuitan edited the summary of this revision. (Show Details)

Fix NativeRegisterContextDBReg_x86

yinghuitan edited the summary of this revision. (Show Details)Jul 13 2023, 9:16 PM

Add fs_base test to compare with pthread_self()

yinghuitan edited the summary of this revision. (Show Details)Jul 13 2023, 11:02 PM
yinghuitan edited the summary of this revision. (Show Details)

Revert x86_64 changes to reduce amount of changes.

yinghuitan edited the summary of this revision. (Show Details)Jul 17 2023, 5:03 PM

LGTM. Anyone else have any issues?

lldb/source/Plugins/Process/Utility/RegisterInfos_x86_64_with_base_shared.cpp
2–3

fix comment line

Caveat: I have 0 prior knowledge about these registers.

What's the testing story here? I see one for fs_base on a live process but none for gs_base and neither for core files. If one test can hit all the code paths those would hit, then fine, but otherwise this needs more coverage.

Side note: please put a tag in the commit title like [lldb][x86] .... It helps people like me who skim through the logs looking for build problems later.

lldb/source/Plugins/Process/Utility/RegisterInfos_x86_64_with_base.h
72

Has this file been put through clang-format? These lines look off.

Address review comments:

  • clang-format
  • Add coredump tests
  • Add test against gs_base register
yinghuitan retitled this revision from Add fs_base/gs_base support for Linux to [lldb][x86_64] Add fs_base/gs_base support for Linux.Jul 18 2023, 9:24 AM
DavidSpickett added inline comments.Jul 19 2023, 2:49 AM
lldb/test/API/commands/register/register/register_command/TestRegisters.py
613

Update this comment.

lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
635 ↗(On Diff #541597)

Stray line?

647 ↗(On Diff #541597)

Is it possible to make them non-zero? Looks like gs_base is often 0 anyway, so making fs_base non-zero would at least prove that we don't just always return 0, and that we don't get fs_base and gs_base mixed up.

DavidSpickett added inline comments.Jul 19 2023, 2:52 AM
lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
647 ↗(On Diff #541597)

Hmm, I see that this core is created from a very simple file that doesn't use pthreads. And adding pthreads in for all the supported platforms might be a can of worms.

There is a thread_crash test you could tack this onto instead, assuming the cpp threads are built on pthreads. It's not great because the test is really focusing on something else, but it's convenient.

Use coredump fs_base/gs_base values from thread_crash test.

Tests LGTM, thanks.

@clayborg please approve if they look good to you.

clayborg accepted this revision.Jul 20 2023, 2:30 PM

LGTM!

This revision is now accepted and ready to land.Jul 20 2023, 2:30 PM
This revision was landed with ongoing or failed builds.Jul 20 2023, 4:33 PM
This revision was automatically updated to reflect the committed changes.

If you feel it's significant enough, it is worth adding a release note in llvm/docs/ReleaseNotes.rst before/on to the llvm 17 branch is taken.