This gdb_remote_client test looks real nice with this update.

OK, I understand the objc testsuite failures. ObjC has tagged pointers, where the address of an object is either a virtual address, or a big old bag of bits that can contain a small value, like instead of a pointer to an NSObject object, you'll have a uint64_t that can be decoded to represent a small integer value without existing in memory. In ValueObject::GetPointerValue(), I clear non-addressable bits, which destroys the tagged pointer values. In the apple internal version of this, we have some additional code that checks the clang type of the ValueObject to see if pointer auth is used for this type before we clear them. But with TBI/MTE values, we need to clear it regardless of type. The correct fix is to audit the objc type formatters for uses of ValueObject::GetPointerValue and have them fetch an unsigned value if there's any possibility of a tagged pointer being used, this is probably going to be a little bit of work.

This looks good to me.

I'll mark it as accepted, I don't know if Jim or Pavel have any comments. I think this is good.

This all looks good to me. the phab says there's a missing newline at the end of TestXMLRegisterFlags.py.

LGTM. I'm not really advocating to changing m_type to be an enum, but curious how that might fit into the total context of this feature, and if it makes any sense.

LGTM.

Update for Jonas' suggestions.

Add an additional bit to the "don't load non-kexts, non-kernels" addition. I refined this by checking to see if the binary is already registered in the Target, and already has a load address set. If the binary is, then we don't re-register it.

Show an alternate approach, where we check the file types of everything in the kernel+kext list, and if something is not a kernel/kext, don't try to load it at all. v. the first change in DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule where I call !IsKernel() && !obj_macho->IsKext() to detect something in the list which isn't processed the standard way that kexts & the kernel are. Either one would work in this case.

Hm, I've also got some problems with some ObjC tagged pointers in the testsuite, regardless of the FixCodeAddress/FixDataAddress issue. Need to look at this a bit more.

Maybe the right answer is to use FixDataAddress. It's possible someone could make a AArch64 FixCodeAddress that clears the low 2 bits.

Need to figure out something for the armv7 case before I try re-landing.

Updating patch with the final version, barring some fix for the armv7 ubuntu bot and TestDataFormatterSmartArray.py line 229 where it has a c-string pointer that gets its 0th bit cleared,

Clean up the test case - the C file and python file - to address David's feedback and test it more thoroughly. Fix ValueObjectPrinter::PrintValueAndSummaryIfNeeded() to only add the actual=hex if this is a pointer type. I think we're all done with this one.

In D144937#4159324, @jasonmolenda wrote:

In cases where we need to store additional information than a weak pointer to an lldb private object, we traditionally add an Impl class which has the additional member(s) and a shared pointer to the lldb private object that backs the class. e.g. see SBValue's ValueImpl, the definition is in SBValue.cpp.

Hi Dan, I hadn't looked this over very closely but one thing that jumped out is that you're adding a member to SBWatchpoint, and we can't do that, it's an API breaking change. In cases where we need to store additional information than a weak pointer to an lldb private object, we traditionally add an Impl class which has the additional member(s) and a shared pointer to the lldb private object that backs the class. e.g. see SBValue's ValueImpl, the definition is in SBValue.cpp.

Ah, sorry I missed this one, LGTM. Attaching to a process very early in startup during this transition between dyld's is especially difficult to recover from; letting the process run a little extra to get past this is a good workaround.

In D144528#4145610, @bulbazord wrote:

In D144528#4145593, @jasonmolenda wrote:

I'm OK with giving this a try. SectionLoadList::SetSectionLoadAddress specifically notes that there are cases were sections overlap in the virtual address space. All of the binaries in the shared cache have a single LINKEDIT segment that is shared, and each binary's mach-o LC_SYMTAB/LC_DYSYMTAB load command will point to different part of that same LINKEDIT. From lldb's perspective, that means every Module in the shared cache has a LINKEDIT Section that overlaps with all the others.

Updated patch to address David's feedback. Thanks David!

Update to fix David's second round of comments, rewrite the Title/Summary to more accurately reflect what I'm doing in this patch. I think we're about done here, but open to any comments.

LGTM.

If I was going to describe this patch briefly at this point, I would say: "Change lldb from depending on the remote gdb stub to tell it whether watchpoints are received before or after the instruction, to knowing the architectures where watchpoint exceptions are received before the instruction executes, and allow the remote gdb stub to override this behavior in its qHostInfo response". The rest is mechanical change.

Went over the rewrite one more time, fixing up comments, fixing a couple of logic bugs I wrote, and having tested it with a debugserver I hacked to remove the watchpoint_exceptions_received:before; key in the qHostInfo reply packet, so lldb had to use its knowledge of the target system to determine this.

Update patch for Jonas' suggestions.

I phabracated wrong and the end of the last msg was truncated. I meant to end it with this:

David's feedback was spot on, overhaul patch to address.

In D143232#4101396, @bulbazord wrote:

LGTM. I wonder if there's a good way to exercise this with a test? Like maybe we can create some bogus unwind information and see if LLDB falls over when consuming it?

tl;dr: watchpoints don't work on things like arm targets with a JTAG gdb stub that doesn't support qWatchpointSupportInfo: or qHostInfo with the watchpoint_exceptions_received key.

Ah, I wasn't paying close enough attention and got the logic for MIPS/PPC64 watchpoint exceptions backwards in my update to GDBRemoteCommunicationClient::GetWatchpointReportedAfter, update patch for that.

hasn't landed it, hasn't beaten me!!!

Fix the nits Jonas pointed out, thanks Jonas.

Thanks for the nits, good points.

Yeah this is a good cleanup, but fwiw maybe because using git, but I sometimes expect break set --help etc to work, forgetting which style lldb uses. just throwing it out there as something I get tripped up by once in a while, not really related to this change, but I can appreciate where the original author thought they were going with it.

Thanks for the feedback Jonas, updated patch.

Add a comment about the intentional placement of binary loading/load address setting in DidLaunchOrAttach to address Jim's feedback.

In D68655#4047126, @labath wrote:

In D68655#4045895, @jasonmolenda wrote:

Both have to be written by the dwarf linker to be correct, but only the former is written ONLY by the dwarf linker.

I don't think that's right:

$ clang -c -x c - -o - -gdwarf-aranges -g <<<"void f(){}" | llvm-readelf - --sections | grep aranges
  [ 6] .debug_aranges    PROGBITS        0000000000000000 0000a0 000030 00      0   0  1
  [ 7] .rela.debug_aranges RELA          0000000000000000 000380 000030 18   I 20   6  8

I guess to say it shorter. If I have a dwarf_aranges, that means the dwarf linker created it. And if it created it, surely its at least based off of the subprogram address ranges or the line table -- that is, the text address ranges. If I have a DW_TAG_compile_unit DW_AT_ranges, either the compiler (to the .o file) created it, in which case I really am suspicious of those ranges because the compiler can't know which symbols will end up in the final executable, and the addresses in the ranges were simply translated to the final executable address equivalents. Or it was rewritten by a dwarf linker that parsed the DWARF and knew how to correctly calculate the addresses that correspond to that compile unit.

I know this is all moot because the dSYM-specific patch landed, but I am curious about this part,

In D68655#4045423, @clayborg wrote:

There have also been some bugs in .debug_aranges and some folks want to get rid of .debug_aranges all together and rely only upon the DW_TAG_compile_unit's DW_AT_ranges. Tons of details in this patch:

https://reviews.llvm.org/D136395

So long story short, some people believe we should not produce or use .debug_aranges anymore, and want LLDB changed to ignore it.

LGTM.

LGTM. Jim?

In D141330#4043397, @labath wrote:

In D141330#4037932, @JDevlieghere wrote:

In D141330#4037925, @aprantl wrote:

Should this be true for a fully linked ELF executable, too?

Sounds reasonable, but adding @labath and @DavidSpickett to confirm. This is trivial to extend later.

It sounds like this is depends on whoever produced the file, not on the file format.

Yeah I'd add a comment explaining that this returns true if the DWARF dwarf_aranges accelerator table can be trusted to be complete by lldb. which it is with dsymutil created dSYM files. Otherwise lldb will confirm that every CU mentioned in the debug_info is included in dwarf_aranges, and for any that are not, it will scan the debug_info for that compile unit looking to make sure there aren't any subprograms that were omitted. Anyway, LGTM.

I agree with landing this change.