[Lldb-commits] [PATCH] D71498: Fix ARM32 inferior calls

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Yeah, this looks like it was fun to track down. I'm going to comment on both patches here, since they are really similar...

Overall, I'm not very enthusiastic about the addr_t wrapper -- I'm not sure it would be even possible because that's a part of the public API. It may be possible to have something like lldb_private::addr_t, but I am not sure about the usefulness of that either. Maybe it would make sense to create two helper functions to convert pointers to and from lldb::addr_t "the right way".

As for these patches, instead of piling on casts, we should try to look for other solutions where it makes sense. For instance, in many cases we have parallel printf and formatv apis, and formatv usually does not require any casts (vs. two casts with printf).

In other places you're replacing a reinterpret_cast<addr_t> with two c casts. I guess this was done because two c++ casts were too long (?). In these cases the second cast is not really needed, as uintptr_t->addr_t should convert automatically. I think I'd prefer that we just have a single reinterpret_cast<uintptr_t> instead of two c casts. Then our rule can be "always convert a pointer to uintptr_t". I don't know if there's a clang-tidy check for that, but it sounds like that could be something which could be checked/enforced there...



================
Comment at: lldb/source/Expression/IRMemoryMap.cpp:577-586
   if (lldb_private::Log *log =
           lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS)) {
     LLDB_LOGF(log,
               "IRMemoryMap::WriteMemory (0x%" PRIx64 ", 0x%" PRIx64
               ", 0x%" PRId64 ") went to [0x%" PRIx64 "..0x%" PRIx64 ")",
-              (uint64_t)process_address, (uint64_t)bytes, (uint64_t)size,
-              (uint64_t)allocation.m_process_start,
+              (uint64_t)process_address, (uint64_t)(uintptr_t)bytes,
+              (uint64_t)size, (uint64_t)allocation.m_process_start,
----------------
This could be something `LLDB_LOG(GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS), "({0:x}, {1:x}, {2:x}) went to [{3:x}, {4:x})", process_address, bytes, allocation.m_process_start, allocation.m_process_start + allocation.m_size)`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71498/new/

https://reviews.llvm.org/D71498