jasonmolenda wrote:

I'd describe this patch as handling the case where a remote stub cannot jit 
memory and has a qMemoryRegion info packet which does not specify permissions.  
The patch you reference makes jitted code expressions work, but I think 
IRMemoryMap::FindSpec only needs to find space in the inferior virtual address 
space -- allocate memory -- and that was already working, wasn't it?  Also, 
your before & after packets show that you've got permissions returned for your 
qMemoryRegionInfo responses now, which you don't before -- you won't hit this 
failure because of that change.

One thing I'd say is that this patch is looking for "is this a region with 
read, write, or execute permissions", so it can skip that.  It does this with, 

```
        if (region_info.GetReadable() != MemoryRegionInfo::OptionalBool::eNo ||
            region_info.GetWritable() != MemoryRegionInfo::OptionalBool::eNo ||
            region_info.GetExecutable() !=
                MemoryRegionInfo::OptionalBool::eNo) {
            ret = region_info.GetRange().GetRangeEnd();
 /// keep iterating
```

but `MemoryRegionInfo::OptionalBool` can be `eDontKnow` which it is here.  I 
think this conditional would be more clearly expressed as

```
       bool all_permissions_unknown = region_info.GetReadable() == 
MemoryRegionInfo::OptionalBool::eDontKnow &&
                                                                
region_info.GetWritable() == MemoryRegionInfo::OptionalBool::eDontKnow &&
                                                                 
region_info.GetExecutable() == MemoryRegionInfo::OptionalBool::eDontKnow;
       bool any_permissions_yes = region_info.GetReadable() == 
MemoryRegionInfo::OptionalBool::eYes
                                                        
region_info.GetWritable() == MemoryRegionInfo::OptionalBool::eYes
                                                         
region_info.GetExecutable() ==  MemoryRegionInfo::OptionalBool::eYes;
        if (all_permissions_unknown || any_permissions_yes)
            ret = region_info.GetRange().GetRangeEnd();
 /// keep iterating
```

or more simply, change the while (true) loop to first check if all permissions 
are No in a region,

```
        if (region_info.GetReadable() != MemoryRegionInfo::OptionalBool::eNo &&
            region_info.GetWritable() != MemoryRegionInfo::OptionalBool::eNo &&
            region_info.GetExecutable() !=
                MemoryRegionInfo::OptionalBool::eNo) {
           if (ret + size < region_info.GetRange().GetRangeEnd())
            return ret;
           else 
             ret = region_info.GetRange().GetRangeEnd();
             // keep searching
```



https://github.com/llvm/llvm-project/pull/99045
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to