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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
