[Lldb-commits] [PATCH] D15437: Read macro info from .debug_macro section and use it for expression evaluation.

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

My main concern here is efficient storage. See my inline comment regarding DebugMacroEntryStorage and let me know what you think.


================
Comment at: include/lldb/Symbol/DebugMacros.h:98
@@ +97,3 @@
+    EntryType m_type;
+    uint64_t m_line;
+    ConstString m_str;
----------------
You can probably drop this to uint32_t. No one will have a file with more than 4 billions lines of code. Then this class will pack better. 

================
Comment at: include/lldb/Symbol/DebugMacros.h:100
@@ +99,3 @@
+    ConstString m_str;
+    ConstString m_file;
+    DebugMacrosSP m_debug_macros_sp;
----------------
m_file should be stored as an integer. CompileUnits has a CompileUnits::GetSupportFiles() which is the file list that is the same list from the DWARF line table file table. This doesn't mean that we have to hand it to consumers as an index, see my comments below about DebugMacroEntryStorage

================
Comment at: include/lldb/Symbol/DebugMacros.h:101
@@ +100,3 @@
+    ConstString m_file;
+    DebugMacrosSP m_debug_macros_sp;
+};
----------------
We are going to have a million of these objects and we should really store this as efficiently as possible:
```
DebugMacroEntryStorage
{
    uint32_t m_type:3;
    uint32_t m_line:29;
    uint32_t m_file_idx; // Index to file in CompileUnit::GetSupportFiles() FileSpecList
    ConstString m_str;
};
```
Then you can expand this into an instance of DebugMacroEntry and have that be what you hand out to external users and this DebugMacroEntry can be as big as you want. Can we inline the entries that are currently representing by adding the m_debug_macros_sp here? Do we really have many places in the macros for a file that include the same thing over and over? I would vote we get rid of the m_debug_macros_sp and just inline the entries where we need to unless we have a specific case where this would make us make way too many DebugMacroEntryStorage objects.

================
Comment at: include/lldb/Symbol/DebugMacros.h:137
@@ +136,3 @@
+
+    std::vector<DebugMacroEntryUP> m_macro_entries;
+};
----------------
tberghammer wrote:
> What is the benefit of storing unique pointers in the vector compared to the objects itself?
I would rather see this as a vector of just the structs as well as tberghammer suggests. No need to scatter thousands of objects around the heap if we don't need to.

================
Comment at: source/Expression/ExpressionSourceCode.cpp:213
@@ +212,3 @@
+    StreamString debug_macros_stream;
+    debug_macros_stream << ""; // Start with an empty string.
+    if (StackFrame *frame = exe_ctx.GetFramePtr())
----------------
Remove this as it isn't needed. The StreamString has a std::string and it doesn't need to have anything in it to say it is empty.


http://reviews.llvm.org/D15437