aleksandr.urakov marked 4 inline comments as done. aleksandr.urakov added a comment.
Thanks all for taking a look! In D67347#1666509 <https://reviews.llvm.org/D67347#1666509>, @amccarth wrote: > This patch is pretty large. It might be easier to break it up into a series > of small steps to get there. The bullet points in the CL description might > be a good roadmap for such a break-up. But let's first figure out the right > abstraction points. I'm open to this, just let me know. ================ Comment at: lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp:15 +static const T *TypedRead(const DataExtractor &data_extractor, offset_t &offset, offset_t size = sizeof(T)) { + return static_cast<const T *>(data_extractor.GetData(&offset, size)); +} ---------------- labath wrote: > It doesn't look like this will do the right thing in if host endianness > differs from the target. You should use GetMaxU64 instead. (or given that PE > should be always little-endian (right?), you could also ditch the > DataExtractor and read stuff by casting to llvm::support::ulittleXX_t. I > believe that's how most of PECOFF parsing in llvm works.) I believe that this code is ok: it actually doesn't read a pointer to data, it gets the data read from the file and interprets it like a structure of type `T` (`GetData` returns just a pointer to the data buffer). I think it's safe because `T` can be `RuntimeFunction`, `UnwindInfo` or `UnwindCode`, but all these types are already using `ulittleXX_t` in their definitions. ================ Comment at: lldb/unittests/Symbol/CMakeLists.txt:17 lldbPluginSymbolFileDWARF + lldbPluginObjectFilePECOFF lldbPluginSymbolFileSymtab ---------------- labath wrote: > As the code you're testing lives in ObjectFilePECOFF, it would probably be > better to move this stuff to `unittests/ObjectFile/PECOFF` (new folder). My bad, fixed, thank you! Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67347/new/ https://reviews.llvm.org/D67347 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
