[Lldb-commits] [PATCH] D18379: [JITLoaderGDB] Read jit entry struct manually.

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

See inlined comments.


================
Comment at: source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp:110-119
@@ -80,3 +109,12 @@
 
-}  // anonymous namespace end
+    template <typename T>
+    T
+    Align(const T v, uint8_t bytes)
+    {
+        uint8_t rem = v % bytes;
+        if (rem == 0)
+            return v;
+        else
+            return  v + bytes - rem;
+    }
 
----------------
Unless this align is special and different from other align functions, we should use llvm::alignTo(...).

================
Comment at: source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp:122
@@ +121,3 @@
+    template <typename ptr_t>
+    bool ReadJITEntry(const addr_t &from_addr, Process *process, jit_code_entry<ptr_t> *entry)
+    {
----------------
Why is "from_addr" passed by reference? No one is updating this address and it is const. This should be "const addr_t from_addr" or it should be non const and "from_addr" should be updated in the call to ReadJITEntry().

================
Comment at: source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp:125-126
@@ +124,4 @@
+        ArchSpec::Core core = process->GetTarget().GetArchitecture().GetCore();
+        bool i386_target = ArchSpec::kCore_x86_32_first <= core && core <= ArchSpec::kCore_x86_32_last;
+        uint8_t uint64_align_bytes =  i386_target ? 4 : 8;
+        const size_t data_byte_size = Align(sizeof(ptr_t) * 3, uint64_align_bytes) + sizeof(uint64_t);
----------------
Is this truly for i386 only? Not for all 32 bit architectures?

================
Comment at: source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp:131
@@ +130,3 @@
+        DataBufferHeap data(data_byte_size, 0);
+        addr_t aligned_addr = Align(from_addr, sizeof(ptr_t)); // Playing absolutely safe
+        size_t bytes_read = process->ReadMemory(aligned_addr, data.GetBytes(), data.GetByteSize(), error);
----------------
Do you really want to adjust this here? Shouldn't this actually be:

```
if (from_addr % sizeof(ptr_t))
    return false;
```


http://reviews.llvm.org/D18379