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

[António Afonso via Phabricator via lldb-commits]

aadsm marked 3 inline comments as done.
aadsm 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));
----------------
labath wrote:
> 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.
oh no, this is embarrassing!


================
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,
----------------
labath wrote:
> 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.
odd, pretty sure I just run clang-format-diff (as I don't have auto formatting going on), will double check this.


================
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));
----------------
labath wrote:
> 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.
Can do, I was trying to follow this api though: http://man7.org/linux/man-pages/man3/getauxval.3.html.
I think I'll go with the uint64_t though, I find it odd to return an addr_t because it's not an address, it's a number that happens to be the same size of an address.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62500