Fix ARM32 inferior calls

[jankratochvil]

Summary

echo -e '#include <unistd.h>\nint main(void){\nsync();return 0;}'|./bin/clang -g -x c -;./bin/lldb -o 'file ./a.out' -o 'b main' -o r -o 'p (void)sync()'

Actual:

error: Expression can't be run, because there is no JIT compiled function

Expected:

<nothing, sync() has been executed>

Casting from 32-bit void * to uint64_t requires an intermediate uintptr_t cast otherwise the pointer gets sign-extended:

echo -e '#include <stdio.h>\n#include <stdint.h>\nint main(void){void *p=(void *)0x80000000;unsigned long long ull=(unsigned long long)p;unsigned long long ull2=(unsigned long long)(uintptr_t)p;printf("p=%p ull=0x%llx ull2=0x%llx\\n",p,ull,ull2);return 0;}'|gcc -Wall -m32 -x c -;./a.out 
<stdin>: In function ‘main’:
<stdin>:3:66: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
p=0x80000000 ull=0xffffffff80000000 ull2=0x80000000

With debug output:
Actual:

IRMemoryMap::WriteMemory (0xb6ff8640, 0xffffffffb6f82158, 0x112) went to [0xb6ff8640..0xb6ff86b3)
Code can be run in the target.
Found function, has local address 0xffffffffb6f84000 and remote address 0xffffffffffffffff
Couldn't disassemble function : Couldn't find code range for function _Z12$__lldb_exprPv
Sections:
[0xb6f84000+0x3c]->0xb6ff9020 (alignment 4, section ID 0, name .text)
...
HandleCommand, command did not succeed
error: Expression can't be run, because there is no JIT compiled function

Expected:

IRMemoryMap::WriteMemory (0xb6ff8640, 0xb6faa15c, 0x128) went to [0xb6ff8640..0xb6ff86c3)
IRExecutionUnit::GetRemoteAddressForLocal() found 0xb6fac000 in [0xb6fac000..0xb6fac040], and returned 0xb6ff9020 from [0xb6ff9020..0xb6ff9060].
Code can be run in the target.
Found function, has local address 0xb6fac000 and remote address 0xb6ff9020
Function's code range is [0xb6ff9020+0x40]
...
Function data has contents:
0xb6ff9020: 10 4c 2d e9 08 b0 8d e2 08 d0 4d e2 00 40 a0 e1
...
Function disassembly:
0xb6ff9020: 0xe92d4c10   push   {r4, r10, r11, lr}

Effect of this patch on ARM32:

-=before patch
+=after patch
 ********************
-Unexpected Passing Tests (1):
+Unexpected Passing Tests (3):
+    lldb-api :: commands/expression/char/TestExprsChar.py
+    lldb-api :: commands/expression/ir-interpreter/TestIRInterpreter.py
     lldb-api :: linux/builtin_trap/TestBuiltinTrap.py

 ********************
-Failing Tests (102):
+Failing Tests (43):
-    lldb-api :: commands/expression/argument_passing_restrictions/TestArgumentPassingRestrictions.py
-    lldb-api :: commands/expression/call-function/TestCallStdStringFunction.py
-    lldb-api :: commands/expression/call-function/TestCallStopAndContinue.py
-    lldb-api :: commands/expression/call-function/TestCallUserDefinedFunction.py
-    lldb-api :: commands/expression/call-overridden-method/TestCallOverriddenMethod.py
-    lldb-api :: commands/expression/call-restarts/TestCallThatRestarts.py
-    lldb-api :: commands/expression/char/TestExprsChar.py
-    lldb-api :: commands/expression/class_template_specialization_empty_pack/TestClassTemplateSpecializationParametersHandling.py
-    lldb-api :: commands/expression/context-object/TestContextObject.py
-    lldb-api :: commands/expression/dont_allow_jit/TestAllowJIT.py
-    lldb-api :: commands/expression/entry-bp/TestExprEntryBP.py
-    lldb-api :: commands/expression/expr-in-syscall/TestExpressionInSyscall.py
-    lldb-api :: commands/expression/formatters/TestFormatters.py
-    lldb-api :: commands/expression/inline-namespace/TestInlineNamespace.py
-    lldb-api :: commands/expression/no-deadlock/TestExprDoesntBlock.py
-    lldb-api :: commands/expression/persistent_types/TestNestedPersistentTypes.py
-    lldb-api :: commands/expression/persistent_types/TestPersistentTypes.py
-    lldb-api :: commands/expression/pr35310/TestExprsBug35310.py
-    lldb-api :: commands/expression/radar_9531204/TestPrintfAfterUp.py
-    lldb-api :: commands/expression/radar_9673664/TestExprHelpExamples.py
     lldb-api :: commands/expression/rdar44436068/Test128BitsInteger.py
-    lldb-api :: commands/expression/static-initializers/TestStaticInitializers.py
-    lldb-api :: commands/expression/test/TestExprs.py
-    lldb-api :: commands/expression/timeout/TestCallWithTimeout.py
-    lldb-api :: commands/expression/unwind_expression/TestUnwindExpression.py
-    lldb-api :: commands/expression/xvalue/TestXValuePrinting.py
     lldb-api :: commands/register/register/register_command/TestRegisters.py
     lldb-api :: commands/watchpoints/hello_watchlocation/TestWatchLocation.py
     lldb-api :: commands/watchpoints/multi_watchpoint_slots/TestWatchpointMultipleSlots.py
     lldb-api :: commands/watchpoints/watchpoint_size/TestWatchpointSizes.py
     lldb-api :: functionalities/breakpoint/require_hw_breakpoints/TestRequireHWBreakpoints.py
-    lldb-api :: functionalities/data-formatter/data-formatter-cpp/TestDataFormatterCpp.py
-    lldb-api :: functionalities/data-formatter/data-formatter-synthval/TestDataFormatterSynthVal.py
     lldb-api :: functionalities/load_unload/TestLoadUnload.py
-    lldb-api :: functionalities/load_using_paths/TestLoadUsingPaths.py
-    lldb-api :: functionalities/memory/find/TestMemoryFind.py
-    lldb-api :: functionalities/process_group/TestChangeProcessGroup.py
     lldb-api :: functionalities/return-value/TestReturnValue.py
     lldb-api :: functionalities/show_location/TestShowLocationDwarf5.py
     lldb-api :: functionalities/thread/num_threads/TestNumThreads.py
-    lldb-api :: lang/c/const_variables/TestConstVariables.py
-    lldb-api :: lang/c/function_types/TestFunctionTypes.py
-    lldb-api :: lang/c/shared_lib/TestSharedLib.py
-    lldb-api :: lang/c/strings/TestCStrings.py
-    lldb-api :: lang/c/struct_types/TestStructTypes.py
-    lldb-api :: lang/cpp/auto/TestCPPAuto.py
-    lldb-api :: lang/cpp/call-function/TestCallCPPFunction.py
-    lldb-api :: lang/cpp/chained-calls/TestCppChainedCalls.py
-    lldb-api :: lang/cpp/class-template-parameter-pack/TestClassTemplateParameterPack.py
-    lldb-api :: lang/cpp/extern_c/TestExternCSymbols.py
-    lldb-api :: lang/cpp/global_operators/TestCppGlobalOperators.py
-    lldb-api :: lang/cpp/lambdas/TestLambdas.py
-    lldb-api :: lang/cpp/llvm-style/TestLLVMStyle.py
-    lldb-api :: lang/cpp/namespace/TestNamespace.py
-    lldb-api :: lang/cpp/namespace/TestNamespaceLookup.py
-    lldb-api :: lang/cpp/namespace_conflicts/TestNamespaceConflicts.py
-    lldb-api :: lang/cpp/namespace_definitions/TestNamespaceDefinitions.py
-    lldb-api :: lang/cpp/operators/TestCppOperators.py
-    lldb-api :: lang/cpp/overloaded-functions/TestOverloadedFunctions.py
-    lldb-api :: lang/cpp/rvalue-references/TestRvalueReferences.py
-    lldb-api :: lang/cpp/scope/TestCppScope.py
-    lldb-api :: lang/cpp/static_methods/TestCPPStaticMethods.py
-    lldb-api :: lang/cpp/symbols/TestSymbols.py
-    lldb-api :: lang/cpp/template-function/TestTemplateFunctions.py
-    lldb-api :: lang/cpp/template/TestTemplateArgs.py
-    lldb-api :: lang/cpp/this/TestCPPThis.py
     lldb-api :: lang/cpp/trivial_abi/TestTrivialABI.py
-    lldb-api :: lang/cpp/unicode-literals/TestUnicodeLiterals.py
-    lldb-api :: lang/cpp/virtual/TestVirtual.py
     lldb-api :: python_api/thread/TestThreadAPI.py
     lldb-api :: tools/lldb-server/TestLldbGdbServer.py
     lldb-shell :: ObjectFile/ELF/PT_LOAD-overlap-PT_INTERP.yaml
     lldb-shell :: ObjectFile/ELF/PT_LOAD-overlap-PT_TLS.yaml
     lldb-shell :: ObjectFile/ELF/PT_LOAD-overlap-section.yaml
     lldb-shell :: ObjectFile/ELF/PT_LOAD.yaml
     lldb-shell :: ObjectFile/ELF/PT_TLS-overlap-PT_LOAD.yaml
     lldb-shell :: Reproducer/Functionalities/TestDataFormatter.test
     lldb-shell :: Reproducer/Functionalities/TestImageList.test
     lldb-shell :: Reproducer/Functionalities/TestStepping.test
     lldb-shell :: Reproducer/TestDump.test
     lldb-shell :: Reproducer/TestGDBRemoteRepro.test
     lldb-shell :: Reproducer/TestRelativePath.test
     lldb-shell :: Reproducer/TestReuseDirectory.test
     lldb-shell :: Reproducer/TestSynchronous.test
     lldb-shell :: Reproducer/TestWorkingDir.test
     lldb-shell :: SymbolFile/DWARF/anon_class_w_and_wo_export_symbols.ll
     lldb-shell :: SymbolFile/DWARF/clang-ast-from-dwarf-unamed-and-anon-structs.cpp
-    lldb-shell :: SymbolFile/DWARF/debug-types-expressions.test
     lldb-shell :: SymbolFile/NativePDB/ast-functions.cpp
     lldb-shell :: SymbolFile/NativePDB/ast-methods.cpp
     lldb-shell :: SymbolFile/NativePDB/ast-types.cpp
     lldb-shell :: SymbolFile/NativePDB/bitfields.cpp
     lldb-shell :: SymbolFile/NativePDB/break-by-function.cpp
     lldb-shell :: SymbolFile/NativePDB/break-by-line.cpp
     lldb-shell :: SymbolFile/NativePDB/function-types-builtins.cpp
     lldb-shell :: SymbolFile/NativePDB/function-types-classes.cpp
     lldb-shell :: SymbolFile/NativePDB/global-classes.cpp
     lldb-shell :: SymbolFile/NativePDB/globals-bss.cpp
     lldb-shell :: SymbolFile/NativePDB/globals-fundamental.cpp
     lldb-shell :: SymbolFile/NativePDB/nested-types.cpp
     lldb-shell :: SymbolFile/NativePDB/source-list.cpp
     lldb-shell :: SymbolFile/NativePDB/tag-types.cpp

-********************
-Unresolved Tests (1):
-    lldb-api :: functionalities/thread/concurrent_events/TestConcurrentManyWatchpoints.py

For me even LLDB trunk was failing. And I do not see how the functionality could be affected by my D63540.

[labath]

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...

lldb/source/Expression/IRMemoryMap.cpp

577–586

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)

[omjavaid]

I still seem to get the same issue after applying this patch and D63540.

echo -e '#include <unistd.h>\nint main(void){\nsync();return 0;}'|./bin/clang -g -x c -;./bin/lldb -o 'file ./a.out' -o 'b main' -o r -o 'p (void)sync()'
(lldb) file ./a.out
Current executable set to '/home/omair.javaid/work/lldb/build/armhf/a.out' (arm).
(lldb) b main
Breakpoint 1: where = a.out`main + 20 at <stdin>:3:1, address = 0x000103c4
(lldb) r
Process 33825 stopped

• thread #1, name = 'a.out', stop reason = breakpoint 1.1 frame #0: 0x000103c4 a.out`main at <stdin>:3:1

Process 33825 launched: '/home/omair.javaid/work/lldb/build/armhf/a.out' (arm)
(lldb) p (void)sync()
error: Can't run the expression locally: Interpreter doesn't handle one of the expression's opcodes
(lldb)

I am trying to track it down will update you later today.

[jankratochvil]

In D71498#1785486, @omjavaid wrote:

error: Can't run the expression locally: Interpreter doesn't handle one of the expression's opcodes

OK, I see it is a different error. I will try to reproduce also that one.

[omjavaid]

This issue is being caused by wrong address being written to memory somewhere while single stepping though i have reached the exact problem but the logs seem to suggest it.

LLDB Log without D63540 https://paste.ubuntu.com/p/zdXFrfN4MJ/

LLDB Log without D63540+D71498+D71514 https://paste.ubuntu.com/p/RCcSdpYkRd/

Problem appears immediately after applying D63540 and has no effect even with remaining two patches.

**Look for Correct behavior log without any patches:

0xe4a8d0: tid = 0x8e07: stop info = run-to-address (stop_id = 7)
Turning off notification of new threads while single stepping a thread.

ThreadList::ShouldStop: 1 threads, 1 unsuspended threads
Thread::ShouldStop(0xe4a8d0) for tid = 0x8e07 0x8e07, pc = 0x00000000000102f0
^^^^^^^^ Thread::ShouldStop Begin ^^^^^^^^
Plan stack initial state:

thread #1: tid = 0x8e07:
  Active plan stack:
    Element 0: Base thread plan.
    Element 1: Thread plan to call 0xf77d8ee8
    Element 2: Run to address: 0x00000000000102f0 using breakpoint: -2 -

th1/fr0 with pc value of 0x102f0, symbol name is '_start'
th1/fr0 frame uses EmulateInstructionARM for full UnwindPlan because this is the non-call site unwind plan and this is a zeroth frame
th1/fr0 0x00000000000102f0: CFA=sp +0 =>

vs the wrong behavior here:

0x1cd38e8: tid = 0x90d4: stop info = <NULL> (stop_id = 7)
0x1cd38e8: tid = 0x90d4: stop info = signal SIGSEGV: invalid address (fault address: 0xfe52) (stop_id = 7)
Turning off notification of new threads while single stepping a thread.

ThreadList::ShouldStop: 1 threads, 1 unsuspended threads
Thread::ShouldStop(0x1cd38e8) for tid = 0x90d4 0x90d4, pc = 0x000000000000fe52
^^^^^^^^ Thread::ShouldStop Begin ^^^^^^^^
Plan stack initial state:

thread #1: tid = 0x90d4:
  Active plan stack:
    Element 0: Base thread plan.
    Element 1: Thread plan to call 0xf77d8ee8
    Element 2: Run to address: 0x00000000000102f0 using breakpoint: -2 -

th1/fr0 using architectural default unwind method
th1/fr0 with pc value of 0xfe52, no symbol/function name is known.
0x01CA20E0 Communication::Write (src = 0x5C600CE0, src_len = %llu) connection = 26
0x1c7c968 ConnectionFileDescriptor::Write (src = 0x5c600ce0, src_len = 26)
0x1c7c968 ConnectionFileDescriptor::Write(fd = 5, src = 0x5c600ce0, src_len = 26) => 26 (error = (null))
this = 0x01CA20E0, dst = 0xA37BD440, dst_len = 8192, timeout = 5000000 us, connection = 0x01C7C968
this = 0x01C7C968, timeout = 5000000 us
0x1c7c968 ConnectionFileDescriptor::Read() fd = 5, dst = 0xa37bd440, dst_len = 8192) => 24, error = (null)
th1/fr0 0x000000000000fe52: CFA=sp +0 => pc=lr

[jankratochvil]

Could you check symtabs what symbols are located at:

th1/fr0 with pc value of 0x102f0, symbol name is '_start'

vs.

th1/fr0 with pc value of 0xfe52, no symbol/function name is known.

? Or maybe just attached the main executable as _start=0x102f0 is there I hope.

[omjavaid]

Symbols sizes are being zeroed out

Correct without D63540: https://pastebin.ubuntu.com/p/fKGxFfwyxV/

Incorrect with D63540: https://pastebin.ubuntu.com/p/XBTScYYkhq/

[clayborg]

Seems like it might be nice to have a macro defined in a LLDB header file like lldb-defines.h. Something like:

#define PTR_TO_U64(x) ((uint64_t)(uintptr_t)(x))

Then we can make a unit test to verify it works on all systems. Seem like this issue will pop up all over the place in the LLDB code base.

[clayborg]

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...

For the printf style statement, we can't use just one cast to "uintptr_t" because on 32 bit systems it won't be converted to 64 bit. If we switch for LLDB_LOG() that uses the llvm formats, we are good, but not with printf

lldb/source/Expression/IRExecutionUnit.cpp

351–352

This can probably just be:

JittedFunction(function.getName().str().c_str(), external, (uintptr_t)fun_ptr));

since this is a function call and "fun_ptr" will be correctly converted to a lldb::addr_t

[clayborg]

As I am reading this, I just wanted to send out a note of something else that can cause crashes in ARM/Thumb code. For anyone working with ARM/Thumb on systems that don't use the ARM and Thumb BKPT instruction when setting software breakpoints (like all lldb linux and android flavors IIRC): if you try to overwrite a 32 bit thumb instruction that is a conditional instruction in a Thumb IT instruction with a 16 bit trap or illegal instruction you can crash your program. The issue arises for code like:

0x1000: xx xx         ITTTEE
0x1002: 00 11 22 33   32 bit thumb instruction (if condition)
0x1006: 44 55 66 77   32 bit thumb instruction (if condition)
0x100a: 88 99 aa bb   32 bit thumb instruction (else condition) 
0x100e: cc dd ee ff   32 bit thumb instruction (else condition)

If you try to set a breakpoint at any of the instructions in [0x1002-0x100e) using a 16 bit trap or illegal instruction (I use "bb bb" below for this trap for example purposes), you change the size of the instructions and which instructions are conditional. If we try to write "bb bb" to 0x1002 we now have:

0x1000: xx xx         ITTTEE
0x1002: bb bb         (if condition) the first conditional instruction is now 16 bit instead of 32 bit
0x1004: 22 33 44 55   (if condition) this has the last half of the previous instruction 
0x1008: 66 77 88 99   (else condition) this has the last half of the previous instruction 
0x100c: aa bb         (else condition) this has the last half of the previous instruction 
0x100e: cc dd ee ff   32 bit thumb instruction (NOT conditional anymore)

This will work if using the BKPT instruction only. Sorry for the noise if lldb-server is already using the BKPT instruction. But I just wanted to throw this out there in case this issue if affecting anyone.

[jankratochvil]

In D71498#1786319, @clayborg wrote:

For the printf style statement, we can't use just one cast to "uintptr_t" because on 32 bit systems it won't be converted to 64 bit.

That pointer-to-uint64_t is now used for printf with PRIx64 so if we use pointer-to-uintptr_t we could use PRIxPTR.

[clayborg]

In D71498#1786343, @jankratochvil wrote:

In D71498#1786319, @clayborg wrote:

For the printf style statement, we can't use just one cast to "uintptr_t" because on 32 bit systems it won't be converted to 64 bit.

That pointer-to-uint64_t is now used for printf with PRIx64 so if we use pointer-to-uintptr_t we could use PRIxPTR.

that will work too!

[omjavaid]

In D71498#1786341, @clayborg wrote:

As I am reading this, I just wanted to send out a note of something else that can cause crashes in ARM/Thumb code. For anyone working with ARM/Thumb on systems that don't use the ARM and Thumb BKPT instruction when setting software breakpoints (like all lldb linux and android flavors IIRC): if you try to overwrite a 32 bit thumb instruction that is a conditional instruction in a Thumb IT instruction with a 16 bit trap or illegal instruction you can crash your program. The issue arises for code like:

0x1000: xx xx         ITTTEE
0x1002: 00 11 22 33   32 bit thumb instruction (if condition)
0x1006: 44 55 66 77   32 bit thumb instruction (if condition)
0x100a: 88 99 aa bb   32 bit thumb instruction (else condition) 
0x100e: cc dd ee ff   32 bit thumb instruction (else condition)

If you try to set a breakpoint at any of the instructions in [0x1002-0x100e) using a 16 bit trap or illegal instruction (I use "bb bb" below for this trap for example purposes), you change the size of the instructions and which instructions are conditional. If we try to write "bb bb" to 0x1002 we now have:

0x1000: xx xx         ITTTEE
0x1002: bb bb         (if condition) the first conditional instruction is now 16 bit instead of 32 bit
0x1004: 22 33 44 55   (if condition) this has the last half of the previous instruction 
0x1008: 66 77 88 99   (else condition) this has the last half of the previous instruction 
0x100c: aa bb         (else condition) this has the last half of the previous instruction 
0x100e: cc dd ee ff   32 bit thumb instruction (NOT conditional anymore)

This will work if using the BKPT instruction only. Sorry for the noise if lldb-server is already using the BKPT instruction. But I just wanted to throw this out there in case this issue if affecting anyone.

I do have this thing on my future agenda but it requires wider testing. When BKPT was implemented in lldb-server back when we were initially adding support for arm/linux, we encountered some unsupported behavior which i dont remember exactly that resulted in sticking with legacy behavior.

[jankratochvil]

In D71498#1785458, @labath wrote:

In other places you're replacing a reinterpret_cast<addr_t> with two c casts.

reinterpret_cast and a (c cast) have the same behavior and as c cast is shorter I did prefer it. But I see clang-tidy prefers reinterpret_cast:

warning: C-style casts are discouraged; use static_cast/const_cast/reinterpret_cast [google-readability-casting]

In these cases the second cast is not really needed, as uintptr_t->addr_t should convert automatically.

Yes.

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...

Yes, I have now written such clang-tidy extension as D71707.

[jankratochvil]

lldb/source/Expression/IRMemoryMap.cpp

577–586

I haven't changed the log function, that could be a different patch, that goes too far IMO.

[jankratochvil]

lldb/source/Core/PluginManager.cpp

89 ↗

(On Diff #234935)

The source is a void * and destination is bool (*PluginInitCallback)() so there is no intermediate integer cast needed: Casting pointer to object to void * in C++
It was caught by D71707 as intptr_t is dangerous.

lldb/source/Expression/IRExecutionUnit.cpp

351–352

Used reinterpret_cast instead of a C cast:

warning: C-style casts are discouraged; use static_cast/const_cast/reinterpret_cast [google-readability-casting]

lldb/source/Host/common/HostInfoBase.cpp

250 ↗

(On Diff #234935)

The source is a static bool ComputeSharedLibraryDirectory(FileSpec &file_spec); and destination is void * so there is no intermediate integer cast needed: Casting pointer to object to void * in C++
It was caught by D71707 as intptr_t is dangerous.

lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp

358 ↗

(On Diff #234935)

The source is a clang::NamedDecl *result_decl and destination is uint64_t V.

lldb/source/Plugins/Process/POSIX/CrashReason.cpp

141 ↗

(On Diff #234935)

Destination is always lldb::addr_t.

147 ↗

(On Diff #234935)

Destination is lldb::addr_t.

[labath]

Thanks for tracking these down!

lldb/source/Expression/IRMemoryMap.cpp

577–586

PRIxPTR is fine too, though I wouldn't really consider changing the log method as "going too far". I tend to do that for all non-trivial modifications to log statements that I make...

lldb/source/Host/common/HostInfoBase.cpp

250 ↗

(On Diff #234935)

I seem to recall some compilers complaining (though still accepting) about code which casts function pointers to void*, with this being the only available workaround. However, I can't reproduce this now, so I guess this could be fine..

[jankratochvil]

lldb/source/Host/common/HostInfoBase.cpp

250 ↗

(On Diff #234935)

I had to study it myself now as I also remembered something like that. It is valid complaint for plain C: https://stackoverflow.com/a/12359083/2995591
But it does not apply ("conditionally") for C++: https://stackoverflow.com/a/12360610/2995591
And it is valid for C++ of: clang-8.0.0-3.fc30.x86_64 && gcc-9.2.1-1.fc30.x86_64
So I would think for C++ it is OK now without the (u)intptr_ intermediate cast.

lldb/source/Expression/IRExecutionUnit.cpp

for (llvm::Function &function : *m_module) {
 
     if (!fun_ptr) {
       error.SetErrorToGenericError();
       error.SetErrorStringWithFormat(
           "'%s' was in the JITted module but wasn't lowered",
           function.getName().str().c_str());
       return;
     }
-    m_jitted_functions.push_back(JittedFunction(
-        function.getName().str().c_str(), external, (lldb::addr_t)fun_ptr));
+    m_jitted_functions.push_back(
+        JittedFunction(function.getName().str().c_str(), external,
+                       (lldb::addr_t)(uintptr_t)fun_ptr));
   }
 
   CommitAllocations(process_sp);
   ReportAllocations(*m_execution_engine_up);
 
   // We have to do this after calling ReportAllocations because for the MCJIT,
   // getGlobalValueAddress will cause the JIT to perform all relocations.  That
   // can only be done once, and has to happen after we do the remapping from

lldb/source/Expression/IRMemoryMap.cpp

case eAllocationPolicyProcessOnly:
     if (process_sp) {
       process_sp->WriteMemory(process_address, bytes, size, error);
       if (!error.Success())
         return;
     }
     break;
   }
 
   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,
               (uint64_t)allocation.m_process_start +
                   (uint64_t)allocation.m_size);
   }
 }
 
 void IRMemoryMap::WriteScalarToMemory(lldb::addr_t process_address,
                                       Scalar &scalar, size_t size,
                                       Status &error) {
   error.Clear();
 
   if (size == UINT32_MAX)