LLVM 19 related updates by cgzones · Pull Request #1513 · htop-dev/htop

cgzones · 2024-08-09T16:03:47Z

No description provided.

BenBE

Not really happy with some of the changes. Can you provide a set of compiler messages describing why certain changes were made?

BenBE · 2024-08-09T17:18:43Z

AffinityPanel.c

      char number[16];
      xSnprintf(number, 9, "CPU %d", Settings_cpuId(host->settings, i));
-      unsigned cpu_width = 4 + strlen(number);
+      unsigned cpu_width = 4 + (unsigned) strlen(number);


Suggested change

unsigned cpu_width = 4 + (unsigned) strlen(number);

unsigned cpu_width = 4 + (unsigned)strlen(number);

BenBE · 2024-08-09T17:21:02Z

CommandLine.c

+               char *endptr;
+               errno = 0;
+               unsigned long res = strtoul(username, &endptr, 10);
+               unsigned castRes = (unsigned) res;


Suggested change

unsigned castRes = (unsigned) res;

unsigned castRes = (unsigned)res;

BenBE · 2024-08-09T17:22:19Z

CommandLine.c

+               errno = 0;
+               unsigned long res = strtoul(username, &endptr, 10);
+               unsigned castRes = (unsigned) res;
+               if (*endptr != '\0' || res == ULONG_MAX || errno != 0 || castRes != res) {


Suggested change

if (*endptr != '\0' || res == ULONG_MAX || errno != 0 || castRes != res) {

if (*endptr != '\0' || res == ULONG_MAX || errno != 0 || res > UINT_MAX) {

or

Suggested change

if (*endptr != '\0' || res == ULONG_MAX || errno != 0 || castRes != res) {

if (*endptr != '\0' || res > UINT_MAX || errno != 0) {

@BenBE I would say res >= UINT_MAX because, AFAIK, (uid_t)-1 is invalid.

That's something the current version didn't check for …

BenBE · 2024-08-09T17:23:38Z

DynamicMeter.c

      if (uiName) {
-         int len = strlen(uiName);
+         size_t len = strlen(uiName);
+         assert(len < 32);


Given this is user-controlled via a config file, this should be a runtime check (DiD).

What does the colon in this uiName string do? And may I replace this strlen call with (size_t)(String_strchrnul(uiName, ':') - uiName) ?

BenBE · 2024-08-09T17:26:30Z

Process.c

+         *pCommStart = (int)(tokenBase - cmdline);
+         *pCommEnd = (int)(token - cmdline);


Given that these both refer to string offsets, we actually should switch to size_t* eventually (DiD).

The type after the substraction is technically ptrdiff_t. Casting to int would lose upper bits. Also, it's a signed type. (So size_t won't fit, but maybe ssize_t would.)

Technically yes, but since tokenBase > cmdline and token > cmdline, both are positive and a cast to size_t wouldn't cause trouble with the sign bit. I'd have to check re sentinel values, but that's another concern.

This Process.c specific changes for LLVM 19 compatibility may be superseded by #1588.

BenBE · 2024-08-09T17:38:49Z

linux/LinuxProcessTable.c

         continue;

-      map_inode = fast_strtoull_dec(&readptr, 0);
+      map_inode = (unsigned int) fast_strtoull_dec(&readptr, 0);


Suggested change

map_inode = (unsigned int) fast_strtoull_dec(&readptr, 0);

map_inode = (unsigned int)fast_strtoull_dec(&readptr, 0);

BenBE · 2024-08-09T17:39:22Z

linux/LinuxProcessTable.c

+            return false;
         }
-         continue;
+


No blank line here …

BenBE · 2024-08-09T17:39:43Z

linux/LinuxProcessTable.c

   char buffer[PROC_LINE_LENGTH + 1] = {0};

-   ssize_t oomRead = xReadfileat(procFd, "oom_score", buffer, sizeof(buffer));
+   int oomRead = (int) xReadfileat(procFd, "oom_score", buffer, sizeof(buffer));


Use ssize_t instead.

BenBE · 2024-08-09T17:41:32Z

linux/LinuxProcessTable.c

-      size_t exeLen = process->procExe ? strlen(process->procExe) : 0;
+      int exeLen = process->procExe ? (int) strlen(process->procExe) : 0;


What's wrong with size_t here?

BenBE · 2024-08-09T17:43:50Z

Vector.c

-   unsigned int n = 0;
+bool Vector_countEquals(const Vector* this, size_t expectedCount) {
+   size_t n = 0;
   for (int i = 0; i < this->items; i++) {


Shouldn't this be updated to size_t here too (I know this would add a cast to this->items).

CRT.c

Explorer09 · 2024-08-09T18:13:56Z

CommandLine.c

+               errno = 0;
+               unsigned long res = strtoul(username, &endptr, 10);
+               unsigned castRes = (unsigned) res;
+               if (*endptr != '\0' || res == ULONG_MAX || errno != 0 || castRes != res) {


Before I can suggest a better code for this, I need one behavior clarified:
Is username being an empty string acceptable here?
Because strtoul would not produce an error for the empty string. It would happily return 0 as the result.

Explorer09 · 2024-08-09T18:28:11Z

Row.c

   int columns = width;
-   RichString_appendnWideColumns(str, attr, content, strlen(content), &columns);
+   size_t contentLen = strlen(content);
+   RichString_appendnWideColumns(str, attr, content, (int)MINIMUM(contentLen, 1000), &columns);


I'm not sure I like this construct. If you want the cap the maximum length for a strlen output, then strnlen is there for use.

Update: I've changed my mind on the use of strnlen here. While strnlen can guarantee it never reads past the specified maxlen, strnlen is (theoretically) less performant than strlen when the input string is well formed already.

That doesn't mean I deny the possibility of using strnlen in htop, but the length clamping, if needed, should be done as early as possible, e.g. in Settings.c when reading the strings from config and not long after the config file was read.

Just using size_t for strlen return values would be slightly cheaper than using strnlen.

Explorer09 · 2024-08-09T18:42:47Z

Meter.h

   GraphData drawData;
   int h;
-   int columnWidthCount;      /**< only used internally by the Header */
+   size_t columnWidthCount;      /**< only used internally by the Header */


Can't understand the reason why expanding the bit width of this columnWidthCount to 64. Would it save code size by saving some cast instructions?

Explorer09 · 2024-08-09T18:48:59Z

Row.c


   unsigned long long totalSeconds = totalMicroseconds / 1000000;
-   unsigned long microseconds = totalMicroseconds % 1000000;
+   unsigned int microseconds = totalMicroseconds % 1000000;


When I wrote this piece of Row_printNanoseconds code, I did not assume the unsigned int type would have 32 bits for you (for stricter standard compliance). I know what you are doing, but I would like uint32_t be used here instead of unsigned int.

Also in favour of explicit bit width spec.

Explorer09 · 2024-08-09T18:53:42Z

Row.c

         fraction /= 10;
      }
-      len = xSnprintf(buffer, sizeof(buffer), "%u.%0*lus ", (unsigned int)totalSeconds, width, fraction);
+      len = xSnprintf(buffer, sizeof(buffer), "%u.%0*us ", (unsigned int)totalSeconds, width, fraction);


I would like this (unsigned long) cast in snprintf function be preserved unless you use PRIu32 format instead.

Explorer09 · 2024-08-09T18:55:38Z

linux/LinuxMachine.c

      char* endp;
      unsigned long int id = strtoul(entry->d_name + 3, &endp, 10);
-      if (id == ULONG_MAX || endp == entry->d_name + 3 || *endp != '\0')
+      if (id == UINT_MAX || endp == entry->d_name + 3 || *endp != '\0')


Suggested change

if (id == UINT_MAX || endp == entry->d_name + 3 || *endp != '\0')

if (id >= UINT_MAX || endp == entry->d_name + 3 || *endp != '\0')

Explorer09 · 2024-08-09T19:04:32Z

linux/LinuxProcessTable.c


   char* oomPtr = buffer;
-   uint64_t oom = fast_strtoull_dec(&oomPtr, oomRead);
+   unsigned long oom = fast_strtoul_dec(&oomPtr, oomRead);


We shouldn't use fast_strou*_dec here, as these "fast" versions come with no overflow check.
By looking at the lines below, it seems like the overflow check is needed.

Explorer09 · 2024-08-09T19:10:31Z

linux/LinuxProcessTable.c

         char* endptr;
         unsigned long parsedPid = strtoul(name, &endptr, 10);
-         if (parsedPid == 0 || parsedPid == ULONG_MAX || *endptr != '\0')
+         if (parsedPid == 0 || parsedPid >= INT_MAX || *endptr != '\0')


The logic doesn't look right here. Why would INT_MAX become an invalid PID anyway? If we want to check for overflow, it could be parsedPid > INT_MAX || !errno.

strtoul returns unsigned long, but the result is later assigned to a field of unsigned int. Thus anything greater than UINT_MAX causes an integer overflow. Everything equal to UINT_MAX is the sentinel value for "invalid user". Thus parsedPid >= UINT_MAX to catch both cases at once.

@BenBE This is PID, not UID. Please get the right context. POSIX didn't have a clear limit on the maximum value of PID, or that PID == INT_MAX is always invalid. So I want to play safe here.

Looking at this Stack Exchange question, it look like Linux currently has a hard limit of 4194303 (the maximum PID is one less than the value in /proc/sys/kernel/pid_max). And that no other OS had been using INT_MAX as maximum PID yet, so treating INT_MAX as invalid value may be fine, for now.

By the way, in Unix, pid_t is always a signed type, unlike uid_t. The pid_t is required to be signed as fork(2) returns value in this type and that negative values are reserved for special uses.

Explorer09 · 2024-08-11T10:09:53Z

Since there are changes in this PR that look not trivial. Would you guys mind if I pick some of the easier changes and file a new PR for them? I just hope some of the changes can be merged early.

Explorer09 · 2024-08-11T10:55:43Z

Action.c

+   unsigned int pos = x;
   Settings* settings = st->host->settings;
-   int s = 2;
+   unsigned int s = 2;


I think for this algorithm, it's better for s to be in size_t, and use (size_t)x (upcast) for comparisons below. I think I would submit my propsed change in a separate PR.

Oops. When I re-review this function code just now, I discovered that the use of strlen here was wrong at the start. This should have been "terminal width" instead and use a function similar to wcswidth but working with multi-byte string instead.

BenBE · 2024-08-11T13:10:31Z

Most of the non-trivial changes in this PR boil down to structural refactorings where many fields that denote size values still use int instead of the more appropriate size_t. Unfortunately, if you start this, this goes through the codebase at least once. Having this as a separate PR would be good. Also splitting other non-trivial changes out is much appreciated.

Enabled in LLVM 19

BenBE · 2025-03-12T21:40:11Z

I moved this PR and #1588 to 3.5.0 because 3.4.1 is targeted to be a minor bugfix release with little refactoring where possible. As these two PRs both carry quite some code changes that are hard to justify as minor fixes, these will have to wait a bit. Also I'm not really happy with the amount of change that is introduced in these two PRs. Please check if we can manage these fixes with a little bit more structured approach.

@Explorer09: Since @cgzones is kinda inactive you may continue the changes of this PR in a superseded implementation, unless there's an objection. If you do, please be mindful to the one who has to review the refactoring. ;-)

BenBE added enhancement Extension or improvement to existing feature code quality ♻️ Code quality enhancement build system 🔧 Affects the build system rather then the user experience labels Aug 9, 2024

BenBE added this to the 3.4.0 milestone Aug 9, 2024

BenBE requested changes Aug 9, 2024

View reviewed changes

Explorer09 reviewed Aug 9, 2024

View reviewed changes

Explorer09 reviewed Aug 11, 2024

View reviewed changes

Explorer09 mentioned this pull request Aug 12, 2024

Partial code improvements with respect to Clang 19 warnings #1516

Closed

This was referenced Aug 14, 2024

Drop unused function Process_rowSetPriority() #1520

Merged

Silence clang analyzer warnings #1521

Merged

Explorer09 mentioned this pull request Aug 22, 2024

Graph meter coloring (with GraphData structure rework) #714

Open

BenBE force-pushed the llvm19 branch from e053b6b to d1d5c07 Compare September 20, 2024 20:54

cgzones added 2 commits September 23, 2024 18:31

ci: bump to LLVM 19

b1b804f

Silence -Wshorten-64-to-32 warnings

fbd42e4

Enabled in LLVM 19

BenBE force-pushed the llvm19 branch from d1d5c07 to fbd42e4 Compare September 23, 2024 16:40

fasterit modified the milestones: 3.4.0, 3.4.1 Feb 20, 2025

BenBE modified the milestones: 3.4.1, 3.5.0 Mar 12, 2025

Explorer09 mentioned this pull request Mar 18, 2025

Unicode character support in screen tab names #1642

Draft

Explorer09 mentioned this pull request Mar 31, 2025

CI: Update LLVM repo and use ubuntu-latest for building "unsupported" htop #1635

Merged

	unsigned cpu_width = 4 + (unsigned) strlen(number);
	unsigned cpu_width = 4 + (unsigned)strlen(number);

	unsigned castRes = (unsigned) res;
	unsigned castRes = (unsigned)res;

	if (*endptr != '\0' \|\| res == ULONG_MAX \|\| errno != 0 \|\| castRes != res) {
	if (*endptr != '\0' \|\| res == ULONG_MAX \|\| errno != 0 \|\| res > UINT_MAX) {

	if (*endptr != '\0' \|\| res == ULONG_MAX \|\| errno != 0 \|\| castRes != res) {
	if (*endptr != '\0' \|\| res > UINT_MAX \|\| errno != 0) {

		*pCommStart = (int)(tokenBase - cmdline);
		*pCommEnd = (int)(token - cmdline);

	map_inode = (unsigned int) fast_strtoull_dec(&readptr, 0);
	map_inode = (unsigned int)fast_strtoull_dec(&readptr, 0);

		size_t exeLen = process->procExe ? strlen(process->procExe) : 0;
		int exeLen = process->procExe ? (int) strlen(process->procExe) : 0;

	if (id == UINT_MAX \|\| endp == entry->d_name + 3 \|\| *endp != '\0')
	if (id >= UINT_MAX \|\| endp == entry->d_name + 3 \|\| *endp != '\0')

Uh oh!

Comments

Conversation

cgzones commented Aug 9, 2024

Uh oh!

BenBE left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Explorer09 Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Explorer09 Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Explorer09 Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Explorer09 commented Aug 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenBE commented Aug 11, 2024

Explorer09 Aug 9, 2024 •

edited

Loading

Explorer09 Aug 13, 2024 •

edited

Loading

Explorer09 Aug 9, 2024 •

edited

Loading