This is an archive of the discontinued LLVM Phabricator instance.

Extend D55859 symbols.enable-external-lookup=false for more testcases
ClosedPublic

Authored by jankratochvil on Jun 14 2019, 9:22 AM.

Details

Summary

D55859 has no effect for some of the testcases so this patch extends it even for (all?) other testcases known to me. LLDB was failing when LLDB prints errors reading system debug infos (*-debuginfo.rpm, DWZ-optimized) which should never happen as LLDB testcases should not be affected by system debug infos.

lldb/packages/Python/lldbsuite/test/api/multithreaded/driver.cpp.template is using only SB API which does not expose ModuleList so I had to call HandleCommand() there.

lldb-test.cpp could also use HandleCommand and then there would be no need for ModuleListProperties::SetEnableExternalLookup() but I think it is cleaner with API and not on based on text commands.

Diff Detail

Repository
rL LLVM

Event Timeline

jankratochvil created this revision.Jun 14 2019, 9:22 AM

It looks like lldb-mi has the --source option. Could that be used to set this setting automatically via lit, as it is done with %lldb ?

It looks like lldb-mi has the --source option. Could that be used to set this setting automatically via lit, as it is done with %lldb ?

The problem is it generates additional ^done line

echo 'settings set symbols.enable-external-lookup false' >/tmp/1j;lldb-mi --source /tmp/1j
(gdb)
settings set symbols.enable-external-lookup false
^done
(gdb)

which the scripts do not expect. They already expect such "unexpected" response from a commandline parameter the scripts pass themselves (%t):

# RUN: %lldbmi %t < %s | FileCheck %s
# Check that we have a valid target created via '%lldbmi %t'.
# CHECK: ^done

This would mean adding additional one # CHECK: ^done for the settings set symbols.enable-external-lookup false command which is not present in the script which I hope you agree is worse than adding both that command+its response as I did.
Or did you mean it some different way? Thanks for the review.

labath accepted this revision.Jun 17 2019, 7:18 AM

It looks like lldb-mi has the --source option. Could that be used to set this setting automatically via lit, as it is done with %lldb ?

The problem is it generates additional ^done line

echo 'settings set symbols.enable-external-lookup false' >/tmp/1j;lldb-mi --source /tmp/1j
(gdb)
settings set symbols.enable-external-lookup false
^done
(gdb)

which the scripts do not expect. They already expect such "unexpected" response from a commandline parameter the scripts pass themselves (%t):

# RUN: %lldbmi %t < %s | FileCheck %s
# Check that we have a valid target created via '%lldbmi %t'.
# CHECK: ^done

This would mean adding additional one # CHECK: ^done for the settings set symbols.enable-external-lookup false command which is not present in the script which I hope you agree is worse than adding both that command+its response as I did.
Or did you mean it some different way? Thanks for the review.

Nope, that's exactly what I meant. I think the fact that you're not able to do this is a pretty big flaw in these lldb-mi tests, but I've already kind of given up on lldb-mi, so I don't care either way..

The rest of the changes seem fine to me.

lldb/packages/Python/lldbsuite/test/lldbtest.py
736 ↗(On Diff #204785)
if os.environ.get("NO_LLDBINIT") != "NO":
This revision is now accepted and ready to land.Jun 17 2019, 7:18 AM
jankratochvil marked an inline comment as done.Jun 17 2019, 7:27 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2019, 7:45 AM