[lldb] Make sure the value of `eSymbolContextVariable` is not conflicting with `RESOLVED_FRAME_CODE_ADDR`

[aprantl]

I *think* this is what the code intended, yes.

[jasonmolenda]

I'm not as sure about this. I think the idea was that eSymbolContextEverything would have a value like 0b1111 and so we know only 4 bits (in my example here) are used for the eSymbolContext* enums. StackFrame's m_flags wants to use the bits above that range for RESOLVED_FRAME_CODE_ADDR, RESOLVED_FRAME_ID_SYMBOL_SCOPE, GOT_FRAME_BASE, RESOLVED_VARIABLES, RESOLVED_GLOBAL_VARIABLES flags. So it's setting RESOLVED_FRAME_CODE_ADDR to (in my example) 0b10000, leaving those 4 low bits free to indicate the eSymbolContext values. That is, we would have

0bnnnn0000 start of eSymbolContext range
0bnnnn1111 end of eSymbolContext range
0b0001mmmm RESOLVED_FRAME_CODE_ADDR
0b0010mmmm RESOLVED_FRAME_ID_SYMBOL_SCOPE
0b0011mmmm GOT_FRAME_BASE
0b0100mmmm RESOLVED_VARIABLES
0b0101mmmm RESOLVED_GLOBAL_VARIABLES

The idea is to have two unrelated set of flags in this single m_flags which I am suspicious of how necessary that is, tbh. In a typical big multi threaded GUI app you might have 20-30 threads with 40-50 stack frames, that's like 1500 StackFrames. I think we keep the stack frame list from the previous step when stepping, so twice that. Having an extra uint32_t for 3k StackFrame objects does not concern me.

I agree that having eSymbolContextVariable be outside the range of eSymbolContextEverything breaks this scheme, but I don't think this is the correct fix.

[jasonmolenda]

a simple fix would be to stick the 5 extra flags the top of the 32-bit m_flags and hope that eSymbolContext* doesn't go that high. I wonder if there's some compile time error that could check that eSymbolContextEverything is less than a certain value or something like that.

[aprantl]

I'll defer to @jasonmolenda

[jasonmolenda]

FWIW I think the only change needed to the original patch is to keep using #define RESOLVED_FRAME_CODE_ADDR (uint32_t(eSymbolContextEverything + 1)) but switch to the new eSymbolContextLastItem that is defined. Possibly the comment about what this code is doing could be made a little clearer, because it is definitely not. :)

[akyrtzi]

In D139066#3964353, @jasonmolenda wrote:

FWIW I think the only change needed to the original patch is to keep using #define RESOLVED_FRAME_CODE_ADDR (uint32_t(eSymbolContextEverything + 1)) but switch to the new eSymbolContextLastItem that is defined. Possibly the comment about what this code is doing could be made a little clearer, because it is definitely not. :)

Thank you for the feedback, but it's unclear to me what you mean, are you saying leave the #define RESOLVED_FRAME_CODE_ADDR line unmodified? If yes, what does "switch to the new eSymbolContextLastItem" mean?

[akyrtzi]

In D139066#3964435, @akyrtzi wrote:

In D139066#3964353, @jasonmolenda wrote:

FWIW I think the only change needed to the original patch is to keep using #define RESOLVED_FRAME_CODE_ADDR (uint32_t(eSymbolContextEverything + 1)) but switch to the new eSymbolContextLastItem that is defined. Possibly the comment about what this code is doing could be made a little clearer, because it is definitely not. :)

Thank you for the feedback, but it's unclear to me what you mean, are you saying leave the #define RESOLVED_FRAME_CODE_ADDR line unmodified? If yes, what does "switch to the new eSymbolContextLastItem" mean?

@jasonmolenda 🙏?

[jasonmolenda]

In D139066#3971564, @akyrtzi wrote:

In D139066#3964435, @akyrtzi wrote:

In D139066#3964353, @jasonmolenda wrote:

FWIW I think the only change needed to the original patch is to keep using #define RESOLVED_FRAME_CODE_ADDR (uint32_t(eSymbolContextEverything + 1)) but switch to the new eSymbolContextLastItem that is defined. Possibly the comment about what this code is doing could be made a little clearer, because it is definitely not. :)

Thank you for the feedback, but it's unclear to me what you mean, are you saying leave the #define RESOLVED_FRAME_CODE_ADDR line unmodified? If yes, what does "switch to the new eSymbolContextLastItem" mean?

@jasonmolenda 🙏?

Ah sorry for not replying Argyrios, just to be explicit I was thinking something like

diff --git a/lldb/include/lldb/lldb-enumerations.h b/lldb/include/lldb/lldb-enumerations.h
index c46ef4bf361..9020c0f4d9f 100644
--- a/lldb/include/lldb/lldb-enumerations.h
+++ b/lldb/include/lldb/lldb-enumerations.h
@@ -377,2 +377,5 @@ FLAGS_ENUM(SymbolContextItem){
     eSymbolContextVariable = (1u << 7),
+
+    // Keep this last and up-to-date for what the last enum value is.
+    eSymbolContextLastItem = ((eSymbolContextVariable << 1) - 1u),
 };
diff --git a/lldb/source/Target/StackFrame.cpp b/lldb/source/Target/StackFrame.cpp
index b600ffe9520..a7f87dbb1c5 100644
--- a/lldb/source/Target/StackFrame.cpp
+++ b/lldb/source/Target/StackFrame.cpp
@@ -46,3 +46,7 @@ using namespace lldb_private;
 // context (m_sc) already.
-#define RESOLVED_FRAME_CODE_ADDR (uint32_t(eSymbolContextEverything + 1))
+// RESOLVED_FRAME_CODE_ADDR adds 1 to the value of eSymbolContextLastItem,
+// assuming this is an all-ones value indicating the positions used by
+// eSymbolContext entries.  We want to define some flags outside that range of
+// bits for our use.
+#define RESOLVED_FRAME_CODE_ADDR (uint32_t(eSymbolContextLastItem + 1))
 #define RESOLVED_FRAME_ID_SYMBOL_SCOPE (RESOLVED_FRAME_CODE_ADDR << 1)

(I haven't tested this or anything) what do you think.

[akyrtzi]

I prefer my original version because:

• eSymbolContextLastItem is not an appropriate name for collection of bits, it should be named like eSymbolContextReallyEverything or something, which then makes things a bit confusing due to having multiple "everything" enums.
• I like the code consistency and readability of all RESOLVED_* macros here being shifts by one of the previous bit. When I first read this code I had to stop and think about why RESOLVED_FRAME_CODE_ADDR is different than the rest; "eSymbolContextLastItem << 1" is clearer to understand IMO, particularly in the context of the other macros.

[jasonmolenda]

In D139066#3971697, @akyrtzi wrote:

I prefer my original version because:

• eSymbolContextLastItem is not an appropriate name for collection of bits, it should be named like eSymbolContextReallyEverything or something, which then makes things a bit confusing due to having multiple "everything" enums.
• I like the code consistency and readability of all RESOLVED_* macros here being shifts by one of the previous bit. When I first read this code I had to stop and think about why RESOLVED_FRAME_CODE_ADDR is different than the rest; "eSymbolContextLastItem << 1" is clearer to understand IMO, particularly in the context of the other macros.

You're right. In my mind, I mixed the two of our solutions together and that would not work. Your solution is consistent and will work. Please land your change, sorry for the distraction.

lldb/include/lldb/lldb-enumerations.h

FLAGS_ENUM(SymbolContextItem){
     eSymbolContextEverything = ((eSymbolContextSymbol << 1) - 1u),
     /// Set when \a global or static variable is requested from a
     /// query, or was located in query results.
     /// eSymbolContextVariable is potentially expensive to lookup so
     /// it isn't included in eSymbolContextEverything which stops it
     /// from being used during frame PC lookups and many other
     /// potential address to symbol context lookups.
     eSymbolContextVariable = (1u << 7),
+
+    // Keep this last and up-to-date for what the last enum value is.
+    eSymbolContextLastItem = eSymbolContextVariable,
 };
 LLDB_MARK_AS_BITMASK_ENUM(SymbolContextItem)
 
 FLAGS_ENUM(Permissions){ePermissionsWritable = (1u << 0),
                         ePermissionsReadable = (1u << 1),
                         ePermissionsExecutable = (1u << 2)};
 LLDB_MARK_AS_BITMASK_ENUM(Permissions)
 

lldb/source/Target/StackFrame.cpp

 #include <memory>
 
 using namespace lldb;
 using namespace lldb_private;
 
 // The first bits in the flags are reserved for the SymbolContext::Scope bits
 // so we know if we have tried to look up information in our internal symbol
 // context (m_sc) already.
-#define RESOLVED_FRAME_CODE_ADDR (uint32_t(eSymbolContextEverything + 1))
+#define RESOLVED_FRAME_CODE_ADDR (uint32_t(eSymbolContextLastItem) << 1)
 #define RESOLVED_FRAME_ID_SYMBOL_SCOPE (RESOLVED_FRAME_CODE_ADDR << 1)
 #define GOT_FRAME_BASE (RESOLVED_FRAME_ID_SYMBOL_SCOPE << 1)
 #define RESOLVED_VARIABLES (GOT_FRAME_BASE << 1)
 #define RESOLVED_GLOBAL_VARIABLES (RESOLVED_VARIABLES << 1)
 
 StackFrame::StackFrame(const ThreadSP &thread_sp, user_id_t frame_idx,
                        user_id_t unwind_frame_index, addr_t cfa,
                        bool cfa_is_valid, addr_t pc, StackFrame::Kind kind,