[Lldb-commits] [lldb] [lldb] Remove support and workarounds for Android 4 and older (PR #124047)

[Pavel Labath via lldb-commits]

labath wrote:

> > > that's especially weird because execve() doesn't use $PATH ... it's the members of the exec family that have a 'p' in the name that do.
> > 
> > 
> > It's been a long time, but I'm pretty sure this was due to a test which ran something with an empty environment (some of our tests do that, sometimes deliberately, mostly by accident). It probably wasn't the binary itself that failed to launch, but that it then couldn't find the executable that _it_ was trying to launch. The comment could be incorrect. Maybe it's (ba)sh who inserts `PATH=/bin:/usr/bin` into the environment if the variable is missing?
> 
> it is, but mksh (Android's shell) does that too.
> 
> > Bottom line: I'm pretty sure that the only reason this needs to be there is to make the test suite happy (and maybe not even that, if things have changed since that time). I don't think anyone is running the lldb test suite on android. If you're not planning to start doing that, I think we could delete it.
> 
> even if anyone is, it might be better to delete this hack and then someone should come and hassle me if/when there's a test failure so we can try to work out what (if anything) is Android-specific here. if nothing else, we'll have a better code comment to add next time round :-)

I made a quick test which I think should replicate the thing scenario that this was working around (run `execlp("env")` with an empty environment, and make sure it's able to find the binary in /system/bin/env -- and it seems that it can, at least on the API 33 device I have around. Therefore, I would be in favor of deleting this as well. @brad0, would you like to do the honors?

> > As I recall, we were cherry-picking the fix into all supported kernel branches (and maybe even adding conformance tests -- I'm not sure), so that at least new devices (starting from a new kernel branch) should have it. But yes, it's possible that claim is too optimistic...
> 
> ah, yes, it's coming back to me now --- it was you who wrote most of the excellent tests in https://cs.android.com/android/platform/superproject/main/+/main:bionic/tests/sys_ptrace_test.cpp wasn't it? i _think_ that the relevant tests weren't in CTS before api 28 though? certainly the copyright date for that whole file was 2016, so it can't have been much earlier than that, and certainly wasn't api 21.

Yes, that was me. :) In particular, the `watchpoint_stress` is what was meant to catch this bug. (It's not guaranteed to do that, since the bug only triggers when the device goes to sleep, and that won't happen if it's connected to usb all the time)

It's quite possible that didn't run as a part of the CTS from the start. If you say it didn't, then I'll take your word for it.

> 
> (fyi, these tests found a linux kernel virtualization bug just recently: https://lore.kernel.org/lkml/Z5FVfe9RwVNr2PGI@google.com/ --- so thank you again for those!)

Wow. Thank *you* for following up on that. I've been aware of issues with running the lldb test suite (the watchpoint tests in particular) in virtualized environments (this is the reason why the lldb linux bot runs on a physical machine and not in the cloud), but I never took the time to isolate and reproduce the bug. It's very cool to see this fixed.

https://github.com/llvm/llvm-project/pull/124047