[NFC][lldb] Adapt LLDB to RemoteInspection's RemoteAddress changes #10928
Conversation
augusto2112
force-pushed
the
change-remote-addr
branch
2 times, most recently
from
10ccd54 to
7ebf13b
Compare
augusto2112
changed the title
1 similar comment
| auto address = swift::remote::RemoteAddress( | ||
| pointer, swift::remote::RemoteAddress::DefaultAddressSpace); |
There was a problem hiding this comment.
could the second parameter have a default value of swift::remote::RemoteAddress::DefaultAddressSpace? The name does suggest it.
There was a problem hiding this comment.
partially related: a few of the RemoteAddress constructors in this patch use a literal zero instead of DefaultAddressSpace, what's the reason for that?
There was a problem hiding this comment.
could the second parameter have a default value of swift::remote::RemoteAddress::DefaultAddressSpace?
I was thinking about making it a default constructor, but I think we want to make it explicit that the address space is something that they think about. I'm worried that if someone updates code in the remote inspection library that grabs the address data from a remote address, threads it to some function, and then builds a new RemoteAddress, they may accidentally change the address space. This pattern of RemoteAddress -> StoredPointer -> RemoteAddress is pretty common, so I want to make it harder to make this mistake.
partially related: a few of the RemoteAddress constructors in this patch use a literal zero instead of DefaultAddressSpace, what's the reason for that?
I just forgot to update those to use the DefaultAddressSpace after I introduced it.
There was a problem hiding this comment.
grabs the address data from a remote address, threads it to some function, and then builds a new RemoteAddress, they may accidentally change the address space. This pattern of RemoteAddress -> StoredPointer -> RemoteAddress is pretty common, so I want to make it harder to make this mistake.
is there a way to provide a safe way of doing this? for example:
auto updatedAddr = originalAddr.map([](StoredPointer rawPtr) {
// do stuff
return newPtr;
});There was a problem hiding this comment.
I think my main issue is the repetitiveness combined with the same value. Despite it being explicit, I could see the default being accidentally used via copy and paste, muscle memory, or LLM generated code.
There was a problem hiding this comment.
I originally had a fromThis method on RemoteAddress that would construct a new address and preserve its address space, but Adrian suggested that that was too opaque and not explicit enough. After refactoring, I agree with him.
Another common case that I didn't mention is reading some memory and constructing a new address out of one of its fields. Something like:
auto someMetadata = Reader->readSomeMetadata(address);
RemoteAddress newAddress(someMetadata->Field, address.getAddressData());
I don't think this is clearer using a map API:
auto newAddress = address.map([](StoredPointer) {
auto someMetadata = Reader->readSomeMetadata(address);
return someMetata->Field;
};
Even if we did adopt such an API, I still think it'd be dangerous to leave the constructor defaulting the address space to the default address space, I think the first thing people would look for when trying to build a new address is its constructor, and they might ignore alternative methods of building the type, and most likely would just write it as:
auto someMetadata = Reader->readSomeMetadata(address);
RemoteAddress newAddress(someMetadata->Field);
| if (pointerSize == 4) { | ||
| uint32_t ptrauthMask32 = 0; | ||
| if (queryDataLayout(DataLayoutQueryType::DLQ_GetPtrAuthMask, nullptr, | ||
| &ptrauthMask32)) | ||
| return (uint64_t)ptrauthMask32; |
There was a problem hiding this comment.
does ptr auth exist for 4 byte pointer systems?
There was a problem hiding this comment.
it doesn't have to be part of this PR, but this code makes me think the query API of DLQ_GetPtrAuthMask should be always uint64_t, regardless of pointer size. It makes code like this more direct, since no call to DLQ_GetPointerSize would be needed. For pointer size of 4, the top 4 bytes of the returned value will always be zero and any code that is specific to 32bit can convert as necessary.
augusto2112
force-pushed
the
change-remote-addr
branch
from
7ebf13b to
1bb80ea
Compare
lldb/source/Plugins/LanguageRuntime/Swift/SwiftMetadataCache.cpp
Outdated
Show resolved
Hide resolved
augusto2112
force-pushed
the
change-remote-addr
branch
from
1bb80ea to
ec05631
Compare
augusto2112
force-pushed
the
change-remote-addr
branch
from
ec05631 to
bd192c3
Compare
augusto2112
force-pushed
the
change-remote-addr
branch
from
bd192c3 to
1cc02a4
Compare
augusto2112
force-pushed
the
change-remote-addr
branch
from
1cc02a4 to
22004c0
Compare
Adapts LLDB to the new field in RemoteAddress. This is an NFC that maintains the current behavior. A follow up patch will be introduced later that takes advantage of the new behavior to fix a bug in reading metadata from files instead of the process. rdar://148361743
augusto2112
force-pushed
the
change-remote-addr
branch
from
22004c0 to
12f429a
Compare
|
@swift-ci test |
nate-chandler
merged commit 56bd1dd
into
swiftlang:stable/20240723
Adapts LLDB to the new field in RemoteAddress. This is an NFC that
maintains the current behavior. A follow up patch will be introduced
later that takes advantage of the new behavior to fix a bug in reading
metadata from files instead of the process.
rdar://148361743