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.
Paths
| Differential D147833
[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 I plan on addressing other similar functions in follow up commits.
Diff Detail
Event TimelineComment Actions 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? Comment Actions
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.
Comment Actions
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. Comment Actions 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. Comment Actions
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. Comment Actions
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. Comment Actions
Yeah, that seems like a safer change. Comment Actions
I agree with Jason. Strong +1 on making that a StringRef. Comparing pointers is asking for trouble. This revision is now accepted and ready to land.Apr 11 2023, 10:20 AM Closed by commit rG6ebf1bc66b89: [lldb] Change return type of EventData::GetFlavor (authored by bulbazord). · Explain WhyApr 11 2023, 10:52 AM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 511835 lldb/include/lldb/Breakpoint/Breakpoint.h
lldb/include/lldb/Breakpoint/Watchpoint.h
lldb/include/lldb/Core/DebuggerEvents.h
lldb/include/lldb/Target/Process.h
lldb/include/lldb/Target/Target.h
lldb/include/lldb/Target/Thread.h
lldb/include/lldb/Utility/Event.h
lldb/source/API/SBEvent.cpp
lldb/source/Breakpoint/Breakpoint.cpp
lldb/source/Breakpoint/Watchpoint.cpp
lldb/source/Core/DebuggerEvents.cpp
lldb/source/Target/Process.cpp
lldb/source/Target/Target.cpp
lldb/source/Target/Thread.cpp
lldb/source/Utility/Event.cpp
|
Note: this is probably the wrong flavor string (i.e. a bug) but I'm just preserving existing behavior here, for better or worse.