labath added a comment.
I think this patch for its test coverage.
The thing I'm not so sure about is the verbatim passing of the os-specific
flags. That's nice for the purpose of displaying them to the user, but it can
make things messy for programatic use, particularly if multiple platforms start
using these. What's do you intend to do with these flags? Like how many of them
are you going to actually use, and for what purpose?
================
Comment at: lldb/source/API/SBMemoryRegionInfo.cpp:127
+ Stream &strm = flags.ref();
+ strm.Printf("%s", m_opaque_up->GetFlags().c_str());
+
----------------
`strm << m_opaque_up->GetFlags()`
================
Comment at: lldb/source/Commands/CommandObjectMemory.cpp:1736
+ result.AppendMessageWithFormatv("flags: {0}",
+ range_info.GetFlags().c_str());
+ }
----------------
drop `.c_str()`
================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1308
+ m_mem_region_cache.emplace_back(Info, file_spec);
+ Result = ST;
+ return true;
----------------
Is this needed. Given that the parsing will stop at the first error, `Result`
should always be empty (success) at this point)...
================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1319
+ // Linux kernel since 2.6.14 has /proc/{pid}/smaps
+ // if CONFIG_PROC_PAGE_MONITOR is enabled
+ auto BufferOrError = getProcFile(GetID(), "smaps");
----------------
Just for my own education: Does this mean that we will need to maintain both
branches in perpetuity, as it is always possible to build kernels which don't
have /smaps ?
================
Comment at: lldb/source/Plugins/Process/Utility/LinuxProcMaps.cpp:134
+ // Note: llvm::regex doesn't do character classes
+ RegularExpression expr("^[ ]*([A-za-z0-9_]+)[ ]*:(.*)");
+ if (expr.Execute(line, &matches)) {
----------------
Compiling the regex for each line is pretty wasteful. I don't think a regex is
really needed here. I think you could just split the line on the first `:`
character and check that lhs does not contain spaces.
================
Comment at: lldb/source/Plugins/Process/Utility/LinuxProcMaps.cpp:19
+enum MapKind { eMaps, eSMaps };
+
----------------
omjavaid wrote:
> May be consider converting this into a class enum.
And get rid of the leading `e`.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2498
+ response.PutCString("flags:");
+ response.PutCString(flags_str.c_str());
+ response.PutChar(';');
----------------
drop `.c_str()`
================
Comment at: lldb/source/Target/MemoryRegionInfo.cpp:21
+ while (flags.size()) {
+ flags = flags.drop_while(iswspace);
+ std::tie(flag, flags) = flags.split(' ');
----------------
`flags = flags.ltrim();`
================
Comment at: lldb/unittests/Process/Utility/CMakeLists.txt:5
+ LINK_LIBS
+ lldbPluginProcessLinux
+ )
----------------
Why is this needed?
================
Comment at: lldb/unittests/Process/Utility/CMakeLists.txt:8-9
+
+target_include_directories(LinuxProcMapsTests PRIVATE
+ ${LLDB_SOURCE_DIR}/source/Plugins/Process/Utility)
----------------
Just include the file as `Plugins/Process/Utility/LinuxProcMaps.h`
================
Comment at: lldb/unittests/Process/Utility/LinuxProcMapsTest.cpp:38
+ void SetUp() override {
+ callback = [&](const MemoryRegionInfo &Info, const Status &ST) {
+ if (ST.Success()) {
----------------
`&`-capture here is dangerous. Capture what you need (this?) explicitly.
================
Comment at: lldb/unittests/Process/Utility/LinuxProcMapsTest.cpp:50-51
+ void check_regions(LinuxProcMapsTestParams params) {
+ ASSERT_EQ(std::get<1>(params).size(), regions.size());
+ ASSERT_EQ(std::get<1>(params), regions);
+ ASSERT_EQ(std::get<2>(params), err_str);
----------------
`ASSERT_THAT(std::get<1>(params), testing::ContainerEq(regions));`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87442/new/
https://reviews.llvm.org/D87442
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
