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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
