[Lldb-commits] [PATCH] D100338: Add a setting that enables memory to be read from the file cache instead of process when the section LLDB is reading from is read-only

Mon Apr 12 18:56:59 PDT 2021

jasonmolenda added a comment.

I'm not super convinced on the setting.  It seems like something you'd add some people could work around a mistake we've made here.

I think we have two cases:

Someone calls Target::ReadMemory prefer_file_cache=true on a writable section.  This is an easy mistake for people to make, they assume prefer_file_cache will only prefer the file cache on read-only Sections.

Someone reads Target::ReadMemory with prefer_file_cache=false on a read-only section.  This is *often* a mistake, but there are times where we genuinely want to see raw memory.

Let's think about what the default behavior of Target::ReadMemory should be. The majority use case is that we prefer the file cache if it is a read-only section.  It's a performance bug in almost every case to behave differently, I maintain.  There are times when we want to show actual raw memory to people.  Some naughty programs that manage to modify themselves, we need to see that real naughtiness, not show what the file had and pretend that is what is in memory.

I think Target::ReadMemory should have a force_live_memory argument, not a prefer_file_cache.  It could be the final argument with a default value of force_live_memory==false.  Almost everyone should let Target::ReadMemory check if this address is in a RO-section, and let it get the data from the file cache if so.  If it's not in a RO section, or any section, read from live memory.  And have the force_live_memory arg to override this.

This is a larger set of changes though, and I don't mean to sign anyone up for more than they intended.  But, just thinking of this from a clean slate, I think that's the better design.  Adrian, what do you think?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100338