[Lldb-commits] [PATCH] Don't use a random probe / allocation scheme in the IRMemoryMap

[Zachary Turner]

Thanks for the history, I think it's more clear to me now on why it worked
the way it did.  But I'm still unclear about one thing.  In the old code,
where was it actually checking for a conflict with an *underlying*
allocation?   AFAICT, it's only checking for a conflict with the IR's
allocation map (e.g. by calling IntersectsAllocation).  Didn't this only
just check for a conflict with the another "fake" allocation?

Short of actually replacing LLDB's global allocator with a custom allocator
that we can use to track native allocations, I'm not sure what a good way
is to make sure we don't intersect with underlying allocations.


On Tue, Jul 8, 2014 at 10:54 AM, Sean Callanan <scallanan at apple.com> wrote:

> Zach,
>
> sorry for the very long wait for this review.  My mail filters haven’t
> been set up to promote patch e-mails from the lldb-commits ML to my main
> inbox; I’ll make sure to track them properly going forward.
>
> We want to create allocations that don’t conflict with underlying process
> allocations because of this scenario:
>
>
>    - You’re running on some kind of device that can’t allocate memory
>    (e.g., a dev board or a kernel core)
>    - You run an expression that sets up some kind of data structure and
>    uses pointers to address into it
>    - That expression also needs to use data from the underlying image
>
>
> Now if you haven’t made sure that you aren’t conflicting with underlying
> allocations, you can get the wrong data when you use data from the
> underlying image.  That’s what this code is meant to avoid.
>
> If there’s a better way to do it, then great!  I used to have pointers in
> the IRInterpreter be annotated with their origin but it was error-prone to
> maintain that illusion throughout the whole run of the expression, which
> might store the pointer somewhere (including but not limited to somewhere
> in the underlying process!) and then re-use it.
>
> That said, there are a few problems with the existing algorithm besides
> what you mentioned.
>
>
>    - You could allocate [0x100, 0x200) happily, checking each end, but
>    there’s an allocation in the underlying process at [0x150, 0x160).  If you
>    make certain assumptions about allocation size (e.g., that allocations are
>    in multiples of the page size) you can eliminate this by probing at
>    intervals between the start and end.
>    - The probing costs round-trip times to the underlying process.
>     Worse, on a dev board it can probe locations that should not be read
>    (e.g., mapped registers).
>
>
> For these reasons I’d actually lean toward using something like your
> patch.  But I don’t like the policy of “Don’t use the zero page” – that
> seems like a fairly ad-hoc, target-specific limitation.  I’d prefer to use
> the image list to avoid areas we *know* are mapped.
>
> Sean
>
> On Jun 25, 2014, at 4:15 PM, Zachary Turner <zturner at google.com> wrote:
>
> The current strategy for host allocation is to choose a random address and
> attempt to allocate there, eventually failing if the allocation cannot be
> satisfied.
>
> The C standard only guarantees that RAND_MAX >= 32767, so for platforms
> where this is true allocations will fail with very high probability.   On
> such platforms, you can reproduce this trivially by running lldb, typing
> "expr (3)" and then hitting enter you see a failure.  Failures generally
> happen with a frequency of about 1 failure every 5 evaluations.
>
> I cannot come up with a good reason that the allocations need to look like
> "real" pointers, so this patch changes the allocation scheme to simply jump
> straight to the end and grab a free chunk of memory.
>
> http://reviews.llvm.org/D4300
>
> Files:
>  source/Expression/IRMemoryMap.cpp
>
> Index: source/Expression/IRMemoryMap.cpp
> ===================================================================
> --- source/Expression/IRMemoryMap.cpp
> +++ source/Expression/IRMemoryMap.cpp
> @@ -53,6 +53,8 @@
>     lldb::ProcessSP process_sp = m_process_wp.lock();
>
>     lldb::addr_t ret = LLDB_INVALID_ADDRESS;
> +    if (size == 0)
> +      return ret;
>
>     if (process_sp && process_sp->CanJIT() && process_sp->IsAlive())
>     {
> @@ -66,37 +68,13 @@
>             return ret;
>     }
>
> -    for (int iterations = 0; iterations < 16; ++iterations)
> -    {
> -        lldb::addr_t candidate = LLDB_INVALID_ADDRESS;
> -
> -        switch (target_sp->GetArchitecture().GetAddressByteSize())
> -        {
> -        case 4:
> -            {
> -                uint32_t random_data = rand();
> -                candidate = random_data;
> -                candidate &= ~0xfffull;
> -                break;
> -            }
> -        case 8:
> -            {
> -                uint32_t random_low = rand();
> -                uint32_t random_high = rand();
> -                candidate = random_high;
> -                candidate <<= 32ull;
> -                candidate |= random_low;
> -                candidate &= ~0xfffull;
> -                break;
> -            }
> -        }
> -
> -        if (IntersectsAllocation(candidate, size))
> -            continue;
> -
> -        ret = candidate;
> -
> -        return ret;
> +    // Don't allocate from the zero page.
> +    ret = 0x1000;
> +    if (!m_allocations.empty()) {
> +      auto back = m_allocations.rbegin();
> +      lldb::addr_t addr = back->first;
> +      size_t size = back->second.m_size;
> +      ret = llvm::RoundUpToAlignment(addr+size, 4096);
>     }
>
>     return ret;
> <D4300.10860.patch>_______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140708/1a0b96bc/attachment.html>