[Lldb-commits] [PATCH] D97249: [lldb] Support debugging utility functions

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added inline comments.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp:38
 const char *ClangExpressionSourceCode::g_expression_prefix =
-"#line 1 \"" PREFIX_NAME R"("
+    "#line 1 \"" PREFIX_NAME R"("#line 1
 #ifndef offsetof
----------------
I assume this is to get the proper file name in the final source code. This would in theory also be required for top-level expressions, but as they are not curiously not using ClangExpressionSourceCode they never get this prefix (and are missing all our fancy type definitions here).

Anyway, this change will prompt the ClangModulesDeclVendor (which usually loads Obj-C modules) to also start loading the imported C++ modules (which I believe won't break anything but just cause a bunch of warnings about failing to load modules). See the only other use of `g_prefix_file_name` where we filter out imported modules from the wrapper file.

I think the idea is that the prefix starts the wrapper and then ClangExpressionSourceCode adds some generated prefixes and then we do a `#line` to terminate the wrapper.  Not sure that's the right place for this use case, but just putting it into the `ClangUtilityFunction` where we concat the source code should work (even though then this logic is stuck in the initalizer, but oh well...)


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp:46
+                                           std::string text, std::string name,
+                                           bool debug)
     : UtilityFunction(
----------------
Can you document `debug` (and maybe we could call it `allow_debugging` or `generate_debuginfo` which is how we call that in normal expressions)?


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp:1595
+          const Symbol *symbol = mod_sp->FindFirstSymbolWithNameAndType(
+              g_class_getNameRaw_symbol_name, lldb::eSymbolTypeCode);
           if (symbol && 
----------------
Unrelated?


================
Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:9715
 ScratchTypeSystemClang::CreateUtilityFunction(std::string text,
-                                              std::string name) {
+                                              std::string name, bool debug) {
   TargetSP target_sp = m_target_wp.lock();
----------------
Not sure if that's needed. `ScratchTypeSystem` is bound to a specific target, so this seems like a good central place to read/use the value of the setting. I wouldn't mind the current way, but this is extending the generic TypeSystem interface and that's just always a bit of a pain to change in the future (when we hopefully have more TypeSystems for different langauges).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97249/new/

https://reviews.llvm.org/D97249