Something seems to have happened to reviews.llvm.org.  I can sort of see 
patches, but the interface isn't responding, and I can't actually make comments 
there, so we'll try this way instead.

Your changes look good, the "ReasonableSizeForReading" reasonable.

About the "force_live_memory" thing.  As I understand it, this is really one of 
those "why we can't have nice things" deals that I think we haven't fully 
embraced.  It would be very nice to not have to go to the device for read-only 
loaded sections, since in some situations it can be much faster.  OTOH, the 
security folks are right, silently suppressing these differences hides 
something that, while it almost never happens, matters a lot when it does.  
There's no other reason that I can think of to choose to read read-only 
sections from memory rather than from the binary.

Maybe this is more the sort of thing that should be a policy controlled by a 
Target setting instead of variables passed around and set different ways by 
different functions?  That way security conscious folks can force the always 
read behavior, and everybody else can work a little faster?

Anyway, the patch looks good, and if I could get Phabricator to allow me to 
check "Accept Revision" and click the Submit button, I would do so.

Jim



> On Nov 16, 2021, at 2:34 AM, Pavel Labath via Phabricator 
> <revi...@reviews.llvm.org> wrote:
> 
> labath updated this revision to Diff 387543.
> labath marked 4 inline comments as done.
> labath added a comment.
> 
> New version.
> 
> 
> Repository:
>  rG LLVM Github Monorepo
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D113098/new/
> 
> https://reviews.llvm.org/D113098
> 
> Files:
>  lldb/include/lldb/DataFormatters/FormattersHelpers.h
>  lldb/include/lldb/DataFormatters/StringPrinter.h
>  lldb/include/lldb/Target/Process.h
>  lldb/include/lldb/Target/Target.h
>  lldb/source/DataFormatters/FormattersHelpers.cpp
>  lldb/source/DataFormatters/StringPrinter.cpp
>  lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp
>  lldb/source/Plugins/Language/CPlusPlus/LibStdcpp.cpp
>  lldb/source/Plugins/Language/ObjC/NSString.cpp
>  lldb/source/Target/Process.cpp
>  lldb/source/Target/Target.cpp
>  lldb/test/API/lang/cpp/char8_t/TestCxxChar8_t.py
> 
> <D113098.387543.patch>

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to