[Lldb-commits] [PATCH] D62500: Add support to read aux vector values

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Fri May 31 02:53:49 PDT 2019


labath added inline comments.


================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:91-94
+  DataExtractor auxv_data(m_process ? m_process->GetAuxvData() : DataBufferSP(),
+                          m_process->GetByteOrder(),
+                          m_process->GetAddressByteSize());
+  m_auxv.reset(new AuxVector(auxv_data));
----------------
It looks like we're not guarding the call to `LoadModules` on line 101, so I think we can assume that m_process is not null here (and I don't see why it should be -- it does not make sense to call `DidAttach` if there is no process around).

BTW, your guarding attempt isn't even correct as we'd always call `m_process->GetByteOrder()` and crash if the process was null.


================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:182-185
+  DataExtractor auxv_data(m_process ? m_process->GetAuxvData() : DataBufferSP(),
+                          m_process->GetByteOrder(),
+                          m_process->GetAddressByteSize());
+  m_auxv.reset(new AuxVector(auxv_data));
----------------
same here.


================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:549-550
 
-  if (ModuleSP module_sp = target.GetOrCreateModule(module_spec, 
-                                                    true /* notify */)) {
+  if (ModuleSP module_sp =
+          target.GetOrCreateModule(module_spec, true /* notify */)) {
     UpdateLoadedSections(module_sp, LLDB_INVALID_ADDRESS, m_interpreter_base,
----------------
Whitespace-only change. Did you maybe run clang-format on the whole file instead of just the diff?

It doesn't matter that much here as there isn't many of them, but I just want to make sure you're aware of this in the future.


================
Comment at: lldb/source/Plugins/Process/Utility/AuxVector.cpp:38
 
-AuxVector::AuxVector(Process *process) : m_process(process) {
-  DataExtractor data;
-  Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
-
-  data.SetData(GetAuxvData());
-  data.SetByteOrder(m_process->GetByteOrder());
-  data.SetAddressByteSize(m_process->GetAddressByteSize());
-
-  ParseAuxv(data);
-
-  if (log)
-    DumpToLog(log);
-}
-
-AuxVector::iterator AuxVector::FindEntry(EntryType type) const {
-  for (iterator I = begin(); I != end(); ++I) {
-    if (I->type == static_cast<uint64_t>(type))
-      return I;
-  }
-
-  return end();
+uint64_t AuxVector::GetAuxValue(enum EntryType entry_type) const {
+  auto it = m_auxv_entries.find(static_cast<uint64_t>(entry_type));
----------------
It would be better to return an Optional<uint64_t> (or addr_t maybe ?) instead of using a magic value to mean "not found". It looks like at least some of these entries can validly be zero.


================
Comment at: lldb/source/Plugins/Process/Utility/AuxVector.cpp:50
   log->PutCString("AuxVector: ");
-  for (iterator I = begin(); I != end(); ++I) {
-    log->Printf("   %s [%" PRIu64 "]: %" PRIx64, GetEntryName(*I), I->type,
-                I->value);
+  for (auto it = m_auxv_entries.begin(); it != m_auxv_entries.end(); ++it) {
+    log->Printf("   %s [%" PRIu64 "]: %" PRIx64,
----------------
range-based for ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62500





More information about the lldb-commits mailing list