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