Page MenuHomePhabricator

Remove m_last_file_sp from SourceManager
ClosedPublic

Authored by emrekultursay on Mar 25 2020, 2:57 PM.

Details

Summary

...and replace it with m_last_file_spec instead.

When Source Cache is enabled, the value stored in m_last_file_sp is
already in the Source Cache, and caching it again in SourceManager
brings no extra benefit. All we need is to "remember" the last used
file, and FileSpec can serve the same purpose.

When Source Cache is disabled, the user explicitly requested no caching
of source files, and therefore, m_last_file_sp should NOT be used.

Bug: llvm.org/PR45310

Depends on D76805.

Diff Detail

Event Timeline

emrekultursay created this revision.Mar 25 2020, 2:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2020, 2:57 PM
labath added a subscriber: labath.

It's not clear to me what is the user-visible effect of this. It sounds like there should be one, but I don't know what it is...

lldb/source/Core/SourceManager.cpp
178–179

if(FileSP last_file_sp = GetLastFile())

255–256

Use GetLastFile, since we already have it.

269–270

same here

emrekultursay edited the summary of this revision. (Show Details)Mar 26 2020, 9:38 AM
emrekultursay marked 3 inline comments as done.Mar 26 2020, 9:57 AM

It's not clear to me what is the user-visible effect of this. It sounds like there should be one, but I don't know what it is...

If LLDB will cache FileSP shared pointers, there should be only one place for caching, and it's the Source File Cache, which can be enabled/disabled by the user.
User-facing effect of this change can be found in bug: llvm.org/PR45310

Applied suggestions from labath@

Added test that verifies end-to-end impact of
"setting set use_source_cache false" on Windows.

Cleanup the Python test to make it a bit more readable

labath added inline comments.Mar 30 2020, 4:36 AM
lldb/test/API/commands/settings/use_source_cache/TestUseSourceCache.py
18

Could you remove this decorator? The test is not extremely interesting on non-windows hosts, but the flow should still work, so there's no harm in testing it.

23

technically, this should be hostoslist=no_match(...)

31–71

All of this could be done via a single lldbutil.run_to_source_breakpoint call (that's a fairly new thing, so it's possible the test you based this on is not using it yet).

41

Please avoid modifying the source tree. You can take a look at test/API/source-manager/Makefile to see how to build binaries with sources in the build tree.

87–88

maybe:

if is_cache_enabled:
  self.assertFalse(is_file_removed)

(and similarly for the other case).

lldb/test/API/commands/settings/use_source_cache/header.h
1 ↗(On Diff #253260)

Does this need to be in a separate file? Could you just put it into main.cpp ?

emrekultursay marked 6 inline comments as done.

PTAL. Applied suggested changes, thanks for the review!

Fix formatting.

labath accepted this revision.Apr 1 2020, 3:19 AM

I think this is fine now. Jim, do you have any more thoughts on this patchset?

lldb/test/API/commands/settings/use_source_cache/TestUseSourceCache.py
41

self.assertGreater

This revision is now accepted and ready to land.Apr 1 2020, 3:19 AM
emrekultursay marked an inline comment as done.

use assertGreater instead of assertTrue(x>y)

jingham accepted this revision.Apr 1 2020, 12:03 PM

There was one missing use of GetLastFile (as opposed to doing it by hand.)

Getting the default file is not used in any performance critical way that I'm aware of. It's mostly used for "break set -l" with no "-f" and list with no arguments and commands of that sort. So the extra lookup to get the FileSP from the source manager instead of caching it directly should not cause problems. And if you asked not to cache, you've already decided to pay the cost for that.

LGTM with that trivial fix.

lldb/source/Core/SourceManager.cpp
178–179

Doesn't look like this got converted to GetLastFile.

Fix missing use of GetLastFile

emrekultursay marked an inline comment as done.Apr 1 2020, 1:22 PM

I don't have commit access, can someone with access commit all three approved CLs in this stack?

labath requested changes to this revision.Apr 9 2020, 2:42 AM

I am afraid that the last patch in this series introduces a bunch of failures for me. The failures appear to be more-or-less consistent, but the impacted tests seem pretty random:

lldb-api :: commands/expression/persistent_ptr_update/TestPersistentPtrUpdate.py
lldb-api :: commands/expression/persistent_variables/TestPersistentVariables.py
lldb-api :: commands/watchpoints/hello_watchlocation/TestWatchLocation.py
lldb-api :: commands/watchpoints/hello_watchpoint/TestMyFirstWatchpoint.py
lldb-api :: commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py
lldb-api :: commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandLLDB.py
lldb-api :: commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py
lldb-api :: commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py
lldb-api :: commands/watchpoints/watchpoint_set_command/TestWatchLocationWithWatchSet.py
lldb-api :: functionalities/breakpoint/auto_continue/TestBreakpointAutoContinue.py
lldb-api :: functionalities/breakpoint/breakpoint_command/TestRegexpBreakCommand.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/atomic/TestLibCxxAtomic.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/function/TestLibCxxFunction.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/multimap/TestDataFormatterLibccMultiMap.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/unordered/TestDataFormatterUnordered.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libstdcpp/map/TestDataFormatterStdMap.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libstdcpp/smart_ptr/TestDataFormatterStdSmartPtr.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libstdcpp/tuple/TestDataFormatterStdTuple.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libstdcpp/unique_ptr/TestDataFormatterStdUniquePtr.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libstdcpp/vector/TestDataFormatterStdVector.py
lldb-api :: functionalities/plugins/python_os_plugin/TestPythonOSPlugin.py
lldb-api :: functionalities/signal/handle-abrt/TestHandleAbort.py
lldb-api :: functionalities/type_completion/TestTypeCompletion.py
lldb-api :: lang/c/register_variables/TestRegisterVariables.py
lldb-api :: lang/cpp/class_types/TestClassTypes.py
lldb-api :: python_api/sbvalue_persist/TestSBValuePersist.py
lldb-shell :: Driver/TestSingleQuote.test
lldb-shell :: Settings/TestFrameFormatMangling.test

Reverting the last patch makes the problem go away, but I am not sure the problem is really there. Given that this series exposes a bunch of previously completely unused code, it could be that the problem is completely elsewhere.

Anyway, this needs to be redone somehow.

This revision now requires changes to proceed.Apr 9 2020, 2:42 AM

Fix broken tests.

Thanks for noticing the test breakages Pavel. I fixed the bug, and verified that the tests you mentioned pass. PTAL.

labath accepted this revision.Apr 20 2020, 7:29 AM

Thanks for noticing the test breakages Pavel. I fixed the bug, and verified that the tests you mentioned pass. PTAL.

Everything looks fine now. Thanks for fixing that (it would be nice to mention what the bug was so one doesn't have to dig through the patch history to figure it out). I'm going to commit this shortly.

This revision is now accepted and ready to land.Apr 20 2020, 7:29 AM
This revision was automatically updated to reflect the committed changes.