[Lldb-commits] [PATCH] D133164: Add the ability to show when variables fails to be available when debug info is valid.

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/include/lldb/Target/StackFrame.h:264
   ///     A pointer to a list of variables.
-  VariableList *GetVariableList(bool get_file_globals);
+  VariableList *GetVariableList(bool get_file_globals, Status *error_ptr);
 
----------------
clayborg wrote:
> There are many places that call this function that also don't need to check the error and if we use a Expected<VariableList*>, we need to add a bunch of consumeError(...) code. See all of the call sites where I added a "nullptr" for the "error_ptr" to see why I chose to do it this way to keep the code cleaner. Many places are getting the variable list to look for things or use them for autocomplete.
Ok, I am convinced by the optionalness of the error in this case.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:4161
+  if (command) {
+    if (command->contains(" -gline-tables-only"))
+      return Status("-gline-tables-only enabled, no variable information is "
----------------
clayborg wrote:
> labath wrote:
> > This isn't a particularly reliable way of detecting whether variable information was emitted. For example a command line `clang -gline-tables-only -g2` will in fact produce full debug info and `clang -g1` will not. Could we make that determination based on the presence of actual variable DIEs in the debug info? Perhaps query the index whether it knows of any (global) variable or any type defined within the compile unit?
> This function only gets called when there are no variables in a stack frame at all and checks for reasons why. So if "-gline-tables-only -g2" is used, this function won't get called if there were variables.
> 
> I planned to make a follow up patch that detected no variables in a compile uint by checking the compile unit's abbreviation set to see if it had any DW_TAG_variable or DW_TAG_formal_parameter definitions, and if there weren't any respond with "-gline-tables-only might be enabled....". 
> 
> If we see this option for sure and have the side affect of there being no variables, I would like to user the users know what they can do to fix things if at all possible. 
I get that, but this check is not completely correct in either direction. For example, `clang -g1` will not produce variable information, but this code will not detect it. Same goes for `clang -gmlt`. And I probably missed some other ways one can prevent variable info from being generated. Keeping up with all the changes in clang flags will just be a game of whack-a-mole. If we checked the actual debug info, then we would catch all of these cases, including the (default) case when the compiler did not embed command line information into the debug info.

And I don't think we need to know the precise command line to give meaningful advice to the users. In all of these cases, sticking an extra `-g` at the end of the command line will cause the variables to reappear. If we wanted to, we could also put a link to the lldb web page where we can (at length) describe the various reasons why variables may not be available and how to fix them.


================
Comment at: lldb/test/API/commands/frame/var/TestFrameVar.py:101
+        for s in error_strings:
+            self.assertTrue(s in command_error, 'Make sure "%s" exists in the command error "%s"' % (s, command_error))
+        for s in error_strings:
----------------
just change to `assertIn(s, command_error)`, and then you get the error message for free.


================
Comment at: lldb/test/API/commands/frame/var/TestFrameVar.py:174-175
+        '''
+        self.build(dictionary={'CFLAGS_EXTRAS': '-gline-tables-only'},
+                   env={"RC_DEBUG_OPTIONS": "1"})
+        exe = self.getBuildArtifact("a.out")
----------------
clayborg wrote:
> labath wrote:
> > Why not just pass `-grecord-command-line` in CFLAGS_EXTRAS? I think then you should be able to remove @skipUnlessDarwin from this test...
> I thought I tried that, but I was able to get this to work as suggested. I will change this.
Cool. Can we also remove @skipUnlessDarwin then?


================
Comment at: lldb/test/API/functionalities/archives/Makefile:12
 	$(AR) $(ARFLAGS) $@ $^
-	$(RM) $^
+	# $(RM) $^
 
----------------
I don't know how important this is, but I believe the build was deleting the .o files to ensure that we access the copies within the archive. If you think that's fine, then just delete this line.


================
Comment at: lldb/test/API/functionalities/archives/Makefile:19
 	$(LLVM_AR) $(LLVM_ARFLAGS) $@ $^
 	# Note for thin archive case, we cannot remove c.o
 
----------------
And probably this comment as well, because it doesn't make sense if we don't delete the other files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133164