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

[Siva Chandra via lldb-commits]

sivachandra added a comment.

I have addressed all the comments (with changes or my own comments).

About the problem with dwo, I have sent a mail to the GCC guys asking for guidance. Will fix it when I get clarity on that.

And yes, it would definitely be good if clang can also produce .debug_macro sections.


================
Comment at: include/lldb/Symbol/DebugMacros.h:101
@@ +100,3 @@
+    ConstString m_file;
+    DebugMacrosSP m_debug_macros_sp;
+};
----------------
clayborg wrote:
> 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.
1. I have taken in other changes but did not remove m_debug_macros_sp. 
2. I have removed the file name / file index member completely as we are not using it now anyway!
3. I did not understand the benefit of creating a new class/struct.
4. For a simple source file like in the test, I see a 100+ defines inserted by default. If we inline them, and if a symbol file contains a 100 simple source files, we will be storing 10000+ duplicate entries. If each of the source files actually includes a standard library header, I would image there would be many more common entries exploding the number of duplicate entries we will be storing.

================
Comment at: source/Expression/ExpressionSourceCode.cpp:122
@@ +121,3 @@
+                else
+                    return;
+                break;
----------------
tberghammer wrote:
> Should this function return an error in this case so the caller know the macro stream is only half ready?
The return here does not mean it is "half ready". The function returns from here as macro entries after this appear on or after the current line in the main source file.

================
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.
+
----------------
tberghammer wrote:
> 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).
I have refactored the code around this now. Since the entire header has to be read past to get to the macro entries, I have collected the data in a data-structure. After the refactor, it is only lives on the stack. While I am OK to take it off if you want, I see no harm in keeping it as a data structure.

Also, since the form value is unused anyway, I have now made it dw_form_t.


http://reviews.llvm.org/D15437