This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Change return type of EventData::GetFlavor
ClosedPublic

Authored by bulbazord on Apr 7 2023, 6:45 PM.

Details

Summary

There's no reason these strings need to be in the ConstString
StringPool, they're already string literals with static lifetime.

I plan on addressing other similar functions in follow up commits.

Diff Detail

Event Timeline

bulbazord created this revision.Apr 7 2023, 6:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2023, 6:45 PM
bulbazord requested review of this revision.Apr 7 2023, 6:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2023, 6:45 PM

Am I missing something, how does this work when we have uses like event_data->GetFlavor() == TargetEventData::GetFlavorString() - is this changing from a one-time construction of a ConstString to a construction of a ConstString every time it's called by implicit construction?

Am I missing something, how does this work when we have uses like event_data->GetFlavor() == TargetEventData::GetFlavorString() - is this changing from a one-time construction of a ConstString to a construction of a ConstString every time it's called by implicit construction?

event_data->GetFlavor() == TargetEventData::GetFlavorString() would be just a straight pointer comparison, no ConstString in the picture. These strings don't need to be in the ConstString pool -- they only exist in one place each and are guaranteed to be there for the lifetime of a running lldb process. There are other places in lldb where I'd like to do this, but I figured I'd do it in a few smaller patches rather than one giant patch.

lldb/source/Utility/Event.cpp
183

Note: this is probably the wrong flavor string (i.e. a bug) but I'm just preserving existing behavior here, for better or worse.

Am I missing something, how does this work when we have uses like event_data->GetFlavor() == TargetEventData::GetFlavorString() - is this changing from a one-time construction of a ConstString to a construction of a ConstString every time it's called by implicit construction?

event_data->GetFlavor() == TargetEventData::GetFlavorString() would be just a straight pointer comparison, no ConstString in the picture. These strings don't need to be in the ConstString pool -- they only exist in one place each and are guaranteed to be there for the lifetime of a running lldb process. There are other places in lldb where I'd like to do this, but I figured I'd do it in a few smaller patches rather than one giant patch.

OK, I see. I saw the == and thought something must be constructed to an object for that operator to work, I didn't even think of address comparing constant data c-strings.

idk maybe I'm over-thinking it, but this does make me a little uncomfortable. We have at least one instance where someone did platform_sp->GetPluginName() == "ios-simulator" in ClangExpressionSourceCode.cpp (probably to avoid a dependency on an an apple platform plugin in generic code; the code should undoubtedly be done differently) and the mach-o linker on darwin today will unique identical c-strings from separate compilation units, but I doubt that's a guarantee on Darwin or on other platforms. Platform::GetPluginName is returning a StringRef here, and we naturally see code like this and assume the c-string will be converted to a StringRef or ConstString implicitly for the comparison operator. But if GetPluginName started returning a pointer to const c-string and the strings aren't uniqued, the comparison fails in a tricky to see way.

idk maybe I'm over-thinking it, but this does make me a little uncomfortable. We have at least one instance where someone did platform_sp->GetPluginName() == "ios-simulator" in ClangExpressionSourceCode.cpp

I only looked around for the first instance of this code pattern, there may be another place where we compare identification strings from objects. And I don't feel confident that I can anticipate what will be added to lldb in the future.

idk maybe I'm over-thinking it, but this does make me a little uncomfortable. We have at least one instance where someone did platform_sp->GetPluginName() == "ios-simulator" in ClangExpressionSourceCode.cpp (probably to avoid a dependency on an an apple platform plugin in generic code; the code should undoubtedly be done differently) and the mach-o linker on darwin today will unique identical c-strings from separate compilation units, but I doubt that's a guarantee on Darwin or on other platforms. Platform::GetPluginName is returning a StringRef here, and we naturally see code like this and assume the c-string will be converted to a StringRef or ConstString implicitly for the comparison operator. But if GetPluginName started returning a pointer to const c-string and the strings aren't uniqued, the comparison fails in a tricky to see way.

idk maybe I'm over-thinking it, but this does make me a little uncomfortable. We have at least one instance where someone did platform_sp->GetPluginName() == "ios-simulator" in ClangExpressionSourceCode.cpp

I only looked around for the first instance of this code pattern, there may be another place where we compare identification strings from objects. And I don't feel confident that I can anticipate what will be added to lldb in the future.

Yes, we definitely do that in many places. I see your point though, comparing const c-string pointers can get pretty tricky if somebody refactors this in the future. In that case, instead of GetFlavor returning a const char * or a ConstString, it could return a StringRef and this should be handled, no? Just like GetPluginName() in your example.

Yes, we definitely do that in many places. I see your point though, comparing const c-string pointers can get pretty tricky if somebody refactors this in the future. In that case, instead of GetFlavor returning a const char * or a ConstString, it could return a StringRef and this should be handled, no? Just like GetPluginName() in your example.

Yeah, that seems like a safer change.

Yes, we definitely do that in many places. I see your point though, comparing const c-string pointers can get pretty tricky if somebody refactors this in the future. In that case, instead of GetFlavor returning a const char * or a ConstString, it could return a StringRef and this should be handled, no? Just like GetPluginName() in your example.

I agree with Jason. Strong +1 on making that a StringRef. Comparing pointers is asking for trouble.

bulbazord updated this revision to Diff 511954.Apr 8 2023, 9:01 PM

Change return type to llvm::StringRef

This revision is now accepted and ready to land.Apr 11 2023, 10:20 AM
This revision was automatically updated to reflect the committed changes.