[feat] Add basic support for iOS 6.1 #1828
Conversation
|
While this thing might work, the fact that it's generated suggests we cannot merge it as is. Because there's no human review, and there is a copyright concern (the AI might have copied code from someone else that is unlicensed). Putting the general issue of AI-generated code aside, i see there's a massive style violation. We're not defining or using a macro token like |
There was a problem hiding this comment.
There are several issues with this implementation, in particular with the feature availability checking, which should best be done using configure checks.
Also, given that the proc_taskinfo structure is allocated on the stack, we should be VERY careful, that we are not mixing ABI boundaries with libproc (which doesn't exist for iPhone 6, but might for later models). Thus ensure, that you only ever have this structure defined when using your own code for accessing proc_taskinfo-related stuff OR know you are using the structure definition that comes from the libproc you are calling.
And now for the elephant in the room: While there is no general ban of AI generated code per se, we still require for contributions to include proper attribution for all (non-trivial) source code copied (or adapted) from other places. An example for this in your PR is with the proc_taskinfo struct (and some other stub types you define), but this also includes magic values like IFT_LOOP, RTM_IFINFO2, AT_FDCWD, AT_SYMLINK_NOFOLLOW,
You should be able to provide a valid and qualified Developer Certificate of Origin alongside with your code, which ATM is not the case (due to the insufficiently attributed third-party sources).
|
Thank you @BenBE and @Explorer09 for the very detailed reviews! Let me address them and get back to you. Putting this to Is there unit tests that I can run? |
darwin/Platform.c
Outdated
| #if !TARGET_OS_IPHONE /* iOS 6.1 SDK doesn't have compressor_page_count and external_page_count */ | ||
| natural_t used = vm->active_count + vm->inactive_count + | ||
| vm->speculative_count + vm->wire_count + | ||
| vm->compressor_page_count - vm->purgeable_count - vm->external_page_count; | ||
| mtr->values[MEMORY_METER_USED] = (double)(used - vm->compressor_page_count) * page_K; | ||
| #else | ||
| natural_t used = vm->active_count + vm->inactive_count + | ||
| vm->speculative_count + vm->wire_count - vm->purgeable_count; | ||
| mtr->values[MEMORY_METER_USED] = (double)used * page_K; | ||
| #endif |
There was a problem hiding this comment.
Nonexistent fields should be better checked in configure.
There was a problem hiding this comment.
This overall nesting of ifdef checks is hard to red overall. A better approach might be needed.
|
Please note the instruction regarding rebasing/amending your commits for PRs in the style guide. |
|
Still grinding on it! 💪
I think I’m getting somewhere pointing the compiler to a libproc from another SDK. If that works, we don’t need a code change, and it will just be a matter of documenting what build parameters to use.
Thank you for the continuous discussion today!
|
tslmy
changed the title
|
The entire PR has become too messy for me to review further. There are still coding style violations here and there. But it seems like there are some compatibility fixes that can be cherry-picked early (such as the check of |
|
Dear @Explorer09, Please keep discussion in here civilized. If you don't like AI-generated code: Fine. If someone has no experience with a certain tool: Help them to get to know that tool or point them to resources that provide an introduction or explain necessary concepts. If code doesn't follow the coding style: Explain the issue and provide possible alternatives. BUT: This is no place for attacks or personal vendettas against any particular technology, just because someone chooses to use them. This is voluntary work. Nobody forces you to respond to each and every issue/PR. |
|
I'm sorry you feel this way @Explorer09 . Please take a break and you're welcome back when you feel better. To both of you @Explorer09 and @BenBE : Thanks for the reviews again. Let me put this PR to draft mode again while I'm at it. (I might need to pick this up only at a next weekend, though.) In general, I'm a bit confused about the "code style" that you all have been referring to. I thought those changes are applied automatically, yet I don't see a pre-commit hook or a CI job kicking in. I've been treating the code style doc as "hey don't be surprised if we modify your code this way eventually / don't be offended when a script keeps formatting your code this way", but judging from your comments this isn't the case. Are those code style rules to be followed manually? |
Some code style cannot be automated prior to commit, and our attitude to pull requests is that: it is the OP's responsibility to code in our style when proposing code changes. If someone cannot bother to adapt to our code style, then it's better to maintain htop in their fork and not propose upstream. Significant disruption to code style and/or paradigms can complicate other developers when reviewing your code. And note that I only review htop PRs on the voluntary basis - and it would be unpleasant for me if someone seems to be deliberately wasting my time. |
The code style is not done automatically, as the code base has several aspects that would break any automatic formatting in a way, which causes unnecessary noise. One example is indentation of Same goes for the header includes: As you noticed, there are some situations where the usual alphabetic order just wouldn't work. Thus automatic enforcement just wouldn't work. And minor deviations are acceptable if they are missed or justified. When writing a PR the style guide actually encourages you to fix minor style issues you notice along the way, like the missing NULL check or even the header ordering issue that was present even before your PR. The only thing the style guide asks in this case is to have these changes in separate commits. More on that in a moment. As code style also includes other aspects like how you perform certain operations. For example with the And no: There is no commit hook magically fixing things. Writing well structured code is the responsibility of the developer – even if they decide to support their work with tools like code formatters (like The style guide is as the word implies a guide and it's intentionally written with a lot of room for things that are acceptable. Okay, not for the commits: The usual PR development easily spirals into dozens of commits with advancing things one step and rewinding others several times in a row. This makes the (version control) history terrible to read if left untreated. That's why the style guide asks you to group your commits not by when you changed things, but by the aspects that you changed in order to arrive at your goal. So any PR that's like more than 3 to 5 commits usually needs a VERY good explanation why it splits things into so many steps. The largest PR I can remember was one where I cleaned up a cross-platform feature to align on all platforms. And that one had only about 30 commits, when intermediate versions of that same PR peaked at over 90 commits. And these 30 commits were not the usual "trial-and-error" "back-and-forth" you have while developing, but the exact steps you needed to go to arrive at the new structure by transforming things from its starting point over to the desired code structure. And that's what is meant by the "rebase early, rebase often" in the style guide: Don't document your journey through the maze, but the best path to reach its goal. |
|
@BenBE that totally makes sense to me. Thanks for the very detailed explanation of the rationales. (What you wrote should really be stored in Let me undo my commits, regroup them, and then force-push a cleaner "history". Spoiler alert -- With a Frankenstein-style SDK I pieced together from iOS and macOS SDKs, I was able to make the 
I'll revert some of the changes in this PR soon and provide instructions on how to compose this SDK instead. |
While automated code style fixing tools can correct the line breaking, indents and token spacing, there are many things that would be off the scope of said tools: Variable or function naming, choice on whether to define a constant as a macro or enum, the overall code structure in a header or source file, etc. Therefore, don't expect there would be a tool, AI powered or not, that magically fixes everything. |
tslmy
force-pushed
the
ios6
branch
2 times, most recently
from
0b64c08 to
dc1b54e
Compare
|
Hi all, What a journey! This Frankenstein-style SDK worked better than I expected (🤦 ). It inspired me to use iPhone Simulator's SDK files for this I guess this "simulator SDK will help a lot" is common knowledge to the jailbreak community 🤷 . But when it comes to learning something, later is better than never! :D I appreciate you all guiding me through this weekend hobbyist project of mine, as I'm quite amateur when it comes to |
Compat.c
Outdated
| #endif | ||
|
|
||
| #ifndef AT_SYMLINK_NOFOLLOW | ||
| # define AT_SYMLINK_NOFOLLOW 0x100 |
There was a problem hiding this comment.
How can we be sure every OS define AT_FDCWD and AT_SYMLINK_NOFOLLOW to the same value? Both constant values seem to be internal to the OS.
There was a problem hiding this comment.
That's actually a good point. I simply saw the code above
/* GNU/Hurd does not have PATH_MAX in limits.h */
#ifndef PATH_MAX
# define PATH_MAX 4096
#endifand assumed hardcoding platform-specific constants is acceptable here at htop.
I've surrounded these two constants with #ifdef __APPLE__. That should be just safe enough.
There was a problem hiding this comment.
@tslmy For limit constants it's okay to define it to an arbitrary value as long as it's a common denominator across all OSes.
Not the case for constants that are used for option flags. Even you can define them, chances are the flags won't work, or worse, it might enable some undocumented feature of that OS's API that could wreak havoc.
There was a problem hiding this comment.
Yeah that's fair. For now I'll just leave the two statements guarded by #ifdef __APPLE__. I'll wait and see what CI says (assuming our test suite covers this).
There was a problem hiding this comment.
Worth taking a step back here - the Compat functions using these macros are only used on Linux (same is true for xReadfileat AFAICT). A simpler fix may involve moving these things into the linux/ subdirectory so that they're not compiled at all on on darwin (or anywhere else).
There was a problem hiding this comment.
Can you examine why in iOS 6 AT_FDCWD and AT_SYMLINK_NOFOLLOW are undefined? This doesn't look normal
I think there's an important distinction to make here: iOS the software itself and the iOS SDK.
I believe it is defined in iOS 6's source code itself (as deep down as XNU); it's just that it's not exposed to us developers in its official SDK.
iOS isn't known for its openness back in the 2010s. It doesn't really surprise me that Apple decided to not expose current working directory (AT_FDCWD) to developers.
That said, when it comes to AT_SYMLINK_NOFOLLOW, there's actually a counterpart in iPhoneOS6.1.sdk/usr/include/sys/fcntl.h:
#define O_NOFOLLOW 0x0100 /* don't follow symlinks */The bottom line: reusing the values defined in XNU is still the best idea I have so far 🤷
There was a problem hiding this comment.
I hear you. Honestly I didn't know /proc was a Linux-specific thing (TIL a lot). I'm not very confident dealing with the clean-up just yet -- still on my warm-up reps here. Do you think we can find someone more experienced in FreeBSD & Linux development to look at it, or could you wait till I make another PR dedicated for this next weekend?
There was a problem hiding this comment.
@tslmy leave it with me, I'll see if this approach is feasible tomorrow.
| used += vm->compressor_page_count; | ||
| mtr->values[MEMORY_METER_USED] = (double)(used - vm->compressor_page_count) * page_K; | ||
| #else | ||
| mtr->values[MEMORY_METER_USED] = (double)used * page_K; |
There was a problem hiding this comment.
A separate mtr->values[MEMORY_METER_USED] assignment line doesn't look right.
There was a problem hiding this comment.
Well, there are already two branches. To avoid assigning again, I'll have to introduce a new variable:
@@ -401,6 +401,7 @@
const struct vm_statistics* vm = &dhost->vm_stats;
#endif
double page_K = (double)vm_page_size / (double)1024;
+ double usedPages = 0.0;
mtr->total = dhost->host_info.max_mem / 1024;
#ifdef HAVE_STRUCT_VM_STATISTICS64
@@ -411,13 +412,14 @@
#endif
#ifdef HAVE_STRUCT_VM_STATISTICS64_COMPRESSOR_PAGE_COUNT
used += vm->compressor_page_count;
- mtr->values[MEMORY_METER_USED] = (double)(used - vm->compressor_page_count) * page_K;
+ usedPages = (double)(used - vm->compressor_page_count);
#else
- mtr->values[MEMORY_METER_USED] = (double)used * page_K;
+ usedPages = (double)used;
#endif
#else
- mtr->values[MEMORY_METER_USED] = (double)(vm->active_count + vm->wire_count) * page_K;
+ usedPages = (double)(vm->active_count + vm->wire_count);
#endif
+ mtr->values[MEMORY_METER_USED] = usedPages * page_K;
// mtr->values[MEMORY_METER_SHARED] = "shared memory, like tmpfs and shm"
#ifdef HAVE_STRUCT_VM_STATISTICS64
#ifdef HAVE_STRUCT_VM_STATISTICS64_COMPRESSOR_PAGE_COUNTwhich doesn't look too elegant to me. What do you say?
There was a problem hiding this comment.
I mean, it might be good to examine why there were two branches in the first place. Ideally the formula for counting "used" memory shouldn't differ between 32-bits and 64-bits.
There was a problem hiding this comment.
It looks like Apple Silicon support (731812d), perhaps something to do with Unified Memory if I'm reading it right.
That said, it's not yet clear to me how to unify all three branches together.
|
Turns out the deprecated name ( I've force-pushed again to make use of #if __IPHONE_OS_VERSION_MIN_REQUIRED <= __IPHONE_6_1
as a stricter way to apply the patches. When building for iOS, this macro is needed anyway. I can put this in |
tslmy
force-pushed
the
ios6
branch
2 times, most recently
from
8cf5d91 to
f608416
Compare
Transition Compat.[ch] into the Linux platform directory. Linux is the only platform using this code after several years now so best if its not compiled into all the other platforms htop binary. However, this change is primarily intended to assist with the iOS port. Related: htop-dev#1828
Transition Compat.[ch] into the Linux platform directory. Linux is the only platform using this code after several years now so best if its not compiled into all the other platforms htop binary. However, this change is primarily intended to assist with the iOS port. Related: htop-dev#1828
… memory statistics (e.g., iPhoneOS6.1).
…ace header to work (e.g., iPhoneOS6.1).
4 participants
Tested working on an iPod touch (4th gen) running iOS 6.1.6:
How to build
Prerequisites:
/Volumes/Xcode.brew install automake)brew install ncurses)Prepare the SDK
htopneeds the IOKit framework for probing battery, disk I/O, and GPU statuses. The vanilla iPhoneOS6.1 SDK does not provide headers for IOKit, so we'll have to stitch together a Frankenstein-style SDK for getting around it.Assuming you are at a directory outside of
htopcodebase:Don't unmount the Xcode DMG just yet; we still need the simulator's SDK (for
libproc.h).Build
Assuming you are now in the
htopdirectory, here's the full commands needed to build: