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

[Siva Chandra via lldb-commits]

sivachandra added inline 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;
+    }
 
----------------
clayborg wrote:
> Unless this align is special and different from other align functions, we should use llvm::alignTo(...).
Removed.

================
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)
+    {
----------------
clayborg wrote:
> 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().
Made it a value. Making it a reference was a slip.

================
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);
----------------
clayborg wrote:
> Is this truly for i386 only? Not for all 32 bit architectures?
Per my reading of the arm32, mip32 and x86 (i386) ABI specs, only x86 aligns 8 byte values at 4-byte boundaries. Others align 8 byte values are 8-byte boundaries.

For my testing, I have arm32 and i386 Android devices at hand which validate the changes.

You can correct me if you think I got it wrong or if I have to consider more archs.

================
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);
----------------
clayborg wrote:
> Do you really want to adjust this here? Shouldn't this actually be:
> 
> ```
> if (from_addr % sizeof(ptr_t))
>     return false;
> ```
Replaced this with an assert.


http://reviews.llvm.org/D18379