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

Tamas Berghammer via lldb-commits lldb-commits at lists.llvm.org
Fri Dec 11 04:32:23 PST 2015


tberghammer added a comment.

The high level architecture looks reasonable but there are a few think I would like you to clean up a bit:

- Several of your function uses a raw "new ...()" and then returns a pointer. Please put the data into a smart pointer (usually std::unique_ptr) and return that one instead to eliminate the risk of a memory leak.
- I added a bunch of inline comments. Most of them are minor style issues but there are some more significant ones (e.g. unused data structures)

How much advantage do you expect from this change from the users point of view? Do you think we should try to make clang produce .debug_macros section also?

In case of split dwarf gcc produces several .debug_macro.dwo section for each compile unit what won't work with your current implementation. I haven't checked the spec if gcc-s behavior is correct or not but it might be a good idea to support it (not necessary with this change). If you decide to not support split dwarf then please mark the test as XFAIL(dwo) also


================
Comment at: include/lldb/Symbol/DebugMacros.h:56
@@ +55,3 @@
+
+    ~DebugMacroEntry() { }
+
----------------
(nit): "= default" (for all other similar function also)

================
Comment at: include/lldb/Symbol/DebugMacros.h:131
@@ +130,3 @@
+        else
+            return NULL;
+    }
----------------
(nit): Please use nullptr (for all other similar case also)

================
Comment at: include/lldb/Symbol/DebugMacros.h:137
@@ +136,3 @@
+
+    std::vector<DebugMacroEntryUP> m_macro_entries;
+};
----------------
What is the benefit of storing unique pointers in the vector compared to the objects itself?

================
Comment at: source/Expression/ExpressionSourceCode.cpp:106
@@ +105,3 @@
+void
+AddMacros(const DebugMacros *dm, AddMacroState &state, StreamString &stream)
+{
----------------
(nit): Please mark this function static and move it out from the anonymous namespace

================
Comment at: source/Expression/ExpressionSourceCode.cpp:122
@@ +121,3 @@
+                else
+                    return;
+                break;
----------------
Should this function return an error in this case so the caller know the macro stream is only half ready?

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.cpp:56-58
@@ +55,5 @@
+    {
+        lldb::offset_t new_offset, str_offset;
+        uint64_t line;
+        const char *macro_str;
+
----------------
(nit): Please initialize

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.h:28
@@ +27,3 @@
+
+enum DWARFMacroHeaderFlagMask
+{
----------------
Please move this enum out from the global namespace (preferably into one of the class) as we are currently leaking both the enum name and the enum fields into the global namespace.

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.h:38
@@ +37,3 @@
+public:
+    class Entry
+    {
----------------
(nit): struct

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.h:47
@@ +46,3 @@
+public:
+    uint8_t entry_count;
+    std::map<uint8_t, Entry> entry_map; // Mapping from opcode number to its entry.
----------------
(nit): Member variable names should start with "m_"

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.h:48
@@ +47,3 @@
+    uint8_t entry_count;
+    std::map<uint8_t, Entry> entry_map; // Mapping from opcode number to its entry.
+
----------------
Why do we need this data structure? Currently nobody is using it.
Also should we store DWARFFormValue instead of llvm::dwarf::Form here? It have a lot of feature in it what makes it work with split dwarf (dwo).


http://reviews.llvm.org/D15437





More information about the lldb-commits mailing list