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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:91-92
                 m_process ? m_process->GetID() : LLDB_INVALID_PROCESS_ID);
-
-  m_auxv.reset(new AuxVector(m_process));
+  DataExtractor auxv_data(m_process->GetAuxvData(), m_process->GetByteOrder(),
+                          m_process->GetAddressByteSize());
+  m_auxv = llvm::make_unique<AuxVector>(auxv_data);
----------------
We should change Process::GetAuxvData() to return a DataExtractor to avoid this kind of code.


================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:94
+  m_auxv = llvm::make_unique<AuxVector>(auxv_data);
   if (log)
     log->Printf("DynamicLoaderPOSIXDYLD::%s pid %" PRIu64 " reloaded auxv data",
----------------
Process can't be NULL ever in a DynamicLoader. It should be a reference, but there may have been complications making that happen for some reason since if you have a member variables "Process &m_process;" it has to be initialized in the contructor? Can't remember. Since process owns the dynamic loader plug-in you can assume "m_process" is always valid and not NULL.


================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:181-182
 
-  m_auxv.reset(new AuxVector(m_process));
+  DataExtractor auxv_data(m_process->GetAuxvData(), m_process->GetByteOrder(),
+                          m_process->GetAddressByteSize());
+  m_auxv = llvm::make_unique<AuxVector>(auxv_data);
----------------
We should change Process::GetAuxvData() to return a DataExtractor to avoid this kind of code.


================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:183
+                          m_process->GetAddressByteSize());
+  m_auxv = llvm::make_unique<AuxVector>(auxv_data);
 
----------------
Use std::move?
```
m_auxv = llvm::make_unique<AuxVector>(std::move(auxv_data));
```


================
Comment at: lldb/source/Plugins/Process/Utility/AuxVector.cpp:11
 
-using namespace lldb;
-using namespace lldb_private;
+AuxVector::AuxVector(lldb_private::DataExtractor &data) { ParseAuxv(data); }
 
----------------
Not a big fan of constructors doing parsing work. No way to return an error unless you build that into the AuxVector class (AuxVector::GetError()).


================
Comment at: lldb/source/Plugins/Process/Utility/AuxVector.cpp:13-20
+static bool ReadUInt(lldb_private::DataExtractor &data,
+                     lldb::offset_t *offset_ptr, uint64_t *value) {
   lldb::offset_t saved_offset = *offset_ptr;
-  *value = data.GetMaxU64(offset_ptr, byte_size);
+  // We're not reading an address but an int that could be 32 or 64 bit
+  // depending on the address size, which is what GetAddress does.
+  *value = data.GetAddress(offset_ptr);
   return *offset_ptr != saved_offset;
----------------
Remove this function and just call "data.GetAddress(offset_ptr)"?


================
Comment at: lldb/source/Plugins/Process/Utility/AuxVector.cpp:24-28
+  while (true) {
+    uint64_t type = 0;
+    uint64_t value = 0;
+    if (!ReadUInt(data, &offset, &type) || !ReadUInt(data, &offset, &value))
       break;
----------------
```
const size_t value_type_size = data.GetAddressByteSize() * 2;
while (data.ValidOffsetForDataOfSize(value_type_size)) {
  const uint64_t type = data.GetAddress(&offset);
  const uint64_t value = data.GetAddress(&offset);
  if (type == AUXV_AT_NULL)
    ...
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62500