Re: [hackers] [sbase][PATCH] libutil/random: better random seed
> - *state = ts.tv_nsec; > + *state = (intptr_t) ^ time(NULL) ^ (ts.tv_nsec * 0xAC5533CD); tv_nsec is `long` which is signed. Signed interger overflow is undefined. You need to cast it to `unsigned long` before multiply: ... ^ ((unsigned long)ts.tv_nsec * 0xAC5533CD) - NRK
Re: [hackers] [sbase][PATCH] head: remove useless buffering
> + while (i < n && (c = fgetc(fp)) != EOF) {
> + if (fputc(c, stdout) == EOF)
I don't see this as an improvement. Each one of the fgetc and fputc are
going to go through a mutex lock (along with possibly going through a
call into a dynamic function).
I think the current solution with getline is fine in practice. It's
unlikely for a line to be too big to malloc.
I think a more worthwhile improvement - to head(1) and a couple other
utils - would be to add nul-delim support via `-z`. It's required in
order to correctly deal with all valid filenames in a shell script. And
AFAIK many of the `-z` like option are currently accepted into the next
POSIX draft.
- NRK
Re: [hackers] Re: [sbase] Orphan patches and unsent patches
On Sun, Mar 03, 2024 at 03:05:47PM +0100, Eolien55 wrote: > but should we not rather implement "Debiased Int Mult (t-opt)"? Both should be fine. The integer mul method avoids clz() so that could be a reason to prefer it. > arc4random(3) doesn't uses seeds, which means that I cannot initialize > it wrong. I see, that makes sense. You had the right idea on one of your eariler patch about mixing malloc-ed address and time but the implementation wasn't good. srandom((intptr_t)buf.nlines | time(NULL)); OR isn't a good mixer because it's biased towards 1 (it's also not reversible): 0 | 0 = 0 0 | 1 = 1 1 | 0 = 1 1 | 1 = 1 XOR would be better. But even better would be to hash it before xor-ing. Just a multiply by an large-ish odd number is usually sufficient: seed = ((uintptr_t)malloced_addr * 0xAC5533CD) ^ time(NULL) /* or use nanoseconds */; You could also mix in a stack address and/or the address of a (usually) dynamically linked function the same way. This should be enough to get a decent seed without relying on system specific interfaces. (See also: https://nullprogram.com/blog/2019/04/30/) - NRK
Re: [hackers] Re: [sbase] Orphan patches and unsent patches
On Sun, Mar 03, 2024 at 12:58:20AM +0100, Elie Le Vaillant wrote: > I'm using the web interface to the mailing list to check what has been > sent, and these patches were not sent. The web archive is not reliable and often drops mails. > + * Copied off OpenBSD (original is arc4random_uniform) > + */ > +uint32_t > +random_uniform(uint32_t upper_bound) I'd recommend using the bitmask approach described in here: https://www.pcg-random.org/posts/bounded-rands.html#bitmask-with-rejection-unbiased-apples-method And if you want a portable clz without depending on __builtin_clz() then see: https://graphics.stanford.edu/~seander/bithacks.html#RoundUpPowerOf2 > + r = random(); /* arc4random() is better, but we don't always > have it */ I'm not sure why you think arc4random() is better. I doubt you need any cryptograhic guarantees here. Personally I'd just go with a simple PCG construction. It takes about ~6 lines of code [0], is fast and has good statistical properties. Would also avoid spending time locking/unlocking mutex inside libc for no reason. [0]: https://github.com/imneme/pcg-c-basic/blob/bc39cd76ac3d541e618606bcc6e1e5ba5e5e6aa3/pcg_basic.c#L60-L67 or: https://codeberg.org/NRK/slashtmp/src/branch/master/prng/pcg_32.c - NRK
Re: [hackers] [dmenu][PATCH] minor improvement to the nomatches cache
Hi Hiltjo,
On Thu, Jul 06, 2023 at 07:08:57PM +0200, Hiltjo Posthuma wrote:
> What hash algorithm is this exactly, it looks interesting? I've found it at:
>
> https://github.com/skeeto/hash-prospector
>
> Is that the one?
The "xor-shift multiply" construct itself is fairly popular and can be
found on many hash algorithms (e.g murmur, fnv etc). I *think* it
originates from LGCs, but not sure about that:
https://en.wikipedia.org/wiki/Linear_congruential_generator
The specific multiplier and shift constants are indeed taken from
hash-prospector README, I've updated v2 commit message to include it.
> What's the measured performance improvement and wouldn't it increase
> collisions
> vs a simple linear search?
In case the cache gets filled up, the false-negative rate of either a
linear search or the hash-table will be unpredictable since both of them
use a "dumb" eviction policy.
The old method overwrites whatever came in first and the new one
overwrites whatever was in `h1`. Something like an LRU *might* help in
those cases, but I think it's not worth the trouble.
As for collisions, the new 2x size made collisions rarer. Testing with a
list of ~1.6k emojies I saw only 5 collisions. (The result will
obviously vary depending on your installed fonts).
But this made me realize that it was dumb to throw away the high bits of
the hash. It can be used to probe another slot. With that, I now only
get 1 collision. I've updated the v2 patch with the new logic.
> I'm not sure we should change the assumption for ints to POSIX in dmenu in all
> cases and assume 32-bit int (although a lot of software in practise does).
> But
> this is very pedantic :)
>
> Bit off-topic: recently I tested some software on MS-DOS with the OpenWatcom
> compiler. It had 16-bit int and caught a bug in my code (lower-level parser
> stuff).
Since dmenu already requires POSIX, I didn't think it'd be a problem.
`uint_least32_t` or `uint32_t` can probably be used, but suckless code
base typically seems to avoid .
---
Avoiding wastage and thus being able to 2x the cache size is the *main*
improvement here, IMO.
The hash-table is measurably faster than linear search but it won't be
noticeable by the user (~30ns vs ~130ns, in theory it also avoids
polluting the cache but I doubt that'll matter much either) so I don't
mind dropping the hash-table if there's concerns about it.
But I think avoiding waste and 2x cache size is worthwhile since it
means we can avoid even more unnecessary and expensive XftFontMatch()
calls.
- NRK
>From 7359c32b9612f8b5fe8c4e6ebf2ab1d903eda989 Mon Sep 17 00:00:00 2001
From: NRK
Date: Fri, 9 Jun 2023 21:11:52 +0600
Subject: [PATCH] minor improvement to the nomatches cache
1. use `unsigned int` to store the codepoints, this avoids waste on
common case where `long` is 64bits. and POSIX guarantees `int` to be
at least 32bits so there's no risk of truncation.
2. since switching to `unsigned int` cuts down the memory requirement by
half, double the cache size from 64 to 128.
3. instead of a linear search, use a simple hash-table for O(1) lookups.
to reduce collusions, the both the lower and the higher bits of the
hash are used to probe 2 different slots.
the hash constants are taken form:
https://github.com/skeeto/hash-prospector
---
dmenu.c | 1 -
drw.c | 23 ---
util.h | 1 +
3 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/dmenu.c b/dmenu.c
index 62f1089..40f93e0 100644
--- a/dmenu.c
+++ b/dmenu.c
@@ -22,7 +22,6 @@
/* macros */
#define INTERSECT(x,y,w,h,r) (MAX(0, MIN((x)+(w),(r).x_org+(r).width) -
MAX((x),(r).x_org)) \
* MAX(0, MIN((y)+(h),(r).y_org+(r).height) -
MAX((y),(r).y_org)))
-#define LENGTH(X) (sizeof X / sizeof X[0])
#define TEXTW(X) (drw_fontset_getwidth(drw, (X)) + lrpad)
/* enums */
diff --git a/drw.c b/drw.c
index a58a2b4..78a2b27 100644
--- a/drw.c
+++ b/drw.c
@@ -238,8 +238,8 @@ drw_rect(Drw *drw, int x, int y, unsigned int w, unsigned
int h, int filled, int
int
drw_text(Drw *drw, int x, int y, unsigned int w, unsigned int h, unsigned int
lpad, const char *text, int invert)
{
- int i, ty, ellipsis_x = 0;
- unsigned int tmpw, ew, ellipsis_w = 0, ellipsis_len;
+ int ty, ellipsis_x = 0;
+ unsigned int tmpw, ew, ellipsis_w = 0, ellipsis_len, hash, h0, h1;
XftDraw *d = NULL;
Fnt *usedfont, *curfont, *nextfont;
int utf8strlen, utf8charlen, render = x || y || w || h;
@@ -251,9 +251,7 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned
int h, unsigned int lp
XftResult result;
int charexists = 0, overflow = 0;
/* keep track of a couple codepoints for which we have no match. */
- enum { nomatches_len = 64 };
- static struct { long codepoint[nomatches_len]; unsigned int idx; }
nomatches;
- static uns
[hackers] [dmenu][PATCH] minor improvement to the nomatches cache
1. use `unsigned int` to store the codepoints, this avoids waste on
common case where `long` is 64bits. and POSIX guarantees `int` to be
at least 32bits so there's no risk of truncation.
2. since switching to `unsigned int` cuts down the memory requirement by
half, double the cache size from 64 to 128.
3. instead of a linear search, use a simple hash-table for O(1) lookups.
---
dmenu.c | 1 -
drw.c | 22 +++---
util.h | 1 +
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/dmenu.c b/dmenu.c
index 62f1089..40f93e0 100644
--- a/dmenu.c
+++ b/dmenu.c
@@ -22,7 +22,6 @@
/* macros */
#define INTERSECT(x,y,w,h,r) (MAX(0, MIN((x)+(w),(r).x_org+(r).width) -
MAX((x),(r).x_org)) \
* MAX(0, MIN((y)+(h),(r).y_org+(r).height) -
MAX((y),(r).y_org)))
-#define LENGTH(X) (sizeof X / sizeof X[0])
#define TEXTW(X) (drw_fontset_getwidth(drw, (X)) + lrpad)
/* enums */
diff --git a/drw.c b/drw.c
index a58a2b4..00a6112 100644
--- a/drw.c
+++ b/drw.c
@@ -238,8 +238,8 @@ drw_rect(Drw *drw, int x, int y, unsigned int w, unsigned
int h, int filled, int
int
drw_text(Drw *drw, int x, int y, unsigned int w, unsigned int h, unsigned int
lpad, const char *text, int invert)
{
- int i, ty, ellipsis_x = 0;
- unsigned int tmpw, ew, ellipsis_w = 0, ellipsis_len;
+ int ty, ellipsis_x = 0;
+ unsigned int tmpw, ew, ellipsis_w = 0, ellipsis_len, hash;
XftDraw *d = NULL;
Fnt *usedfont, *curfont, *nextfont;
int utf8strlen, utf8charlen, render = x || y || w || h;
@@ -251,9 +251,7 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned
int h, unsigned int lp
XftResult result;
int charexists = 0, overflow = 0;
/* keep track of a couple codepoints for which we have no match. */
- enum { nomatches_len = 64 };
- static struct { long codepoint[nomatches_len]; unsigned int idx; }
nomatches;
- static unsigned int ellipsis_width = 0;
+ static unsigned int nomatches[128], ellipsis_width;
if (!drw || (render && (!drw->scheme || !w)) || !text || !drw->fonts)
return 0;
@@ -338,11 +336,13 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned
int h, unsigned int lp
* character must be drawn. */
charexists = 1;
- for (i = 0; i < nomatches_len; ++i) {
- /* avoid calling XftFontMatch if we know we
won't find a match */
- if (utf8codepoint == nomatches.codepoint[i])
- goto no_match;
- }
+ hash = (unsigned int)utf8codepoint;
+ hash = ((hash >> 16) ^ hash) * 0x21F0AAAD;
+ hash = ((hash >> 15) ^ hash) * 0xD35A2D97;
+ hash = ((hash >> 15) ^ hash) % LENGTH(nomatches);
+ /* avoid expensive XftFontMatch call when we know we
won't find a match */
+ if (nomatches[hash] == utf8codepoint)
+ goto no_match;
fccharset = FcCharSetCreate();
FcCharSetAddChar(fccharset, utf8codepoint);
@@ -371,7 +371,7 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned
int h, unsigned int lp
curfont->next = usedfont;
} else {
xfont_free(usedfont);
- nomatches.codepoint[++nomatches.idx %
nomatches_len] = utf8codepoint;
+ nomatches[hash] = utf8codepoint;
no_match:
usedfont = drw->fonts;
}
diff --git a/util.h b/util.h
index f633b51..c0a50d4 100644
--- a/util.h
+++ b/util.h
@@ -3,6 +3,7 @@
#define MAX(A, B) ((A) > (B) ? (A) : (B))
#define MIN(A, B) ((A) < (B) ? (A) : (B))
#define BETWEEN(X, A, B)((A) <= (X) && (X) <= (B))
+#define LENGTH(X) (sizeof (X) / sizeof (X)[0])
void die(const char *fmt, ...);
void *ecalloc(size_t nmemb, size_t size);
--
2.41.0
[hackers] [libgrapheme][PATCH] also provide better compressed zst tarball
libgrapheme's tarball, as of master branch, reaches about 2.5M, which is much bigger than most suckless software (although this is still pretty small by modern standards). with zstd's highest compression setting, this can be cut in half. zstd is also included in the base system for many distros - which may be able to make use of this. $ du -h libgrapheme-*tar.* 824Klibgrapheme-2.0.2.tar.gz 472Klibgrapheme-2.0.2.tar.zst 2.5Mlibgrapheme-master.tar.gz 1.2Mlibgrapheme-master.tar.zst --- Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index e403bb2..05e3f44 100644 --- a/Makefile +++ b/Makefile @@ -360,6 +360,7 @@ dist: cp $(SRC:=.c) src/util.h "libgrapheme-$(VERSION)/src" cp $(TEST:=.c) test/util.c test/util.h "libgrapheme-$(VERSION)/test" tar -cf - "libgrapheme-$(VERSION)" | gzip -c > "libgrapheme-$(VERSION).tar.gz" + tar -cf - "libgrapheme-$(VERSION)" | zstd -c --ultra -22 -T0 > "libgrapheme-$(VERSION).tar.zst" rm -rf "libgrapheme-$(VERSION)" format: -- 2.40.1
[hackers] [libgrapheme][PATCH] clean .so file with a glob
otherwise, other versions will end up sticking around. e.g due to `git pull`-ing new changes or checking out a different branch etc. --- And why we're here, why not break up the line? It's pretty long as of now and was kind of annoying to inspect and/or edit. Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index e403bb2..c8ba7a5 100644 --- a/Makefile +++ b/Makefile @@ -342,7 +342,7 @@ uninstall: if ! [ -z "$(PCPREFIX)" ]; then rm -f "$(DESTDIR)$(PCPREFIX)/libgrapheme.pc"; fi clean: - rm -f $(BENCHMARK:=.o) benchmark/util.o $(BENCHMARK:=$(BINSUFFIX)) $(GEN:=.h) $(GEN:=.o) gen/util.o $(GEN:=$(BINSUFFIX)) $(SRC:=.o) src/util.o $(TEST:=.o) test/util.o $(TEST:=$(BINSUFFIX)) $(ANAME) $(SONAME) $(MAN3:=.3) $(MAN7:=.7) + rm -f $(BENCHMARK:=.o) benchmark/util.o $(BENCHMARK:=$(BINSUFFIX)) $(GEN:=.h) $(GEN:=.o) gen/util.o $(GEN:=$(BINSUFFIX)) $(SRC:=.o) src/util.o $(TEST:=.o) test/util.o $(TEST:=$(BINSUFFIX)) $(ANAME) libgrapheme.so.* $(MAN3:=.3) $(MAN7:=.7) clean-data: rm -f $(DATA) -- 2.40.1
Re: [hackers] [libgrapheme] Apply clang format || Laslo Hunhold
On Fri, May 26, 2023 at 06:37:38PM +0200, Quentin Rameau wrote: > > Apply clang format > > Those are quite ugly Assuming you're talking about these commits (and not the formatting itself) - clang-format has a git integration script: https://clang.llvm.org/docs/ClangFormat.html#git-integration You can use a git hook to run it automatically when committing. I don't know the exact details on how to set that up, since I've never done it myself, just know that it's possible to do. - NRK
Re: [hackers] [st][PATCH] Fix xresources bgcolour fgcolour and cscolour definitions
Hi Hari, On Tue, Mar 21, 2023 at 09:29:06PM +0530, Hari Shankar wrote: > I once again apologise in advance as this is my first time making a patch, > sending it through mail or using a mailing list. This list is for submitting patches to be included in mainline code. For community patches (the ones you can see on the website under "patch" section), you can directly push your fixes to the website's git repo: https://git.suckless.org/sites/ But before pushing anything, see this: https://suckless.org/wiki/ - NRK
[hackers] [tabbed][PATCH] fix: faulty zombie reaping
issues with the current signal handler:
1. die() isn't async-signal-safe
2. there's a race-window during reinstating the signal handler allowing
zombies to be produced (should be rare in practice).
3. if waitpid fails, it will clobber the errno which can lead to
undesired codepath being taken on the resuming code (this one can
actually occur in practice).
to reproduce the 3rd issue:
~> ./tabbed &
[1] 20644
0x183
~> while :; do kill -s SIGCHLD 20644; done >&2 2>/dev/null
XIO: fatal IO error 10 (No child processes) on X server ":0"
after 47 requests (47 known processed) with 0 events remaining.
[1] + exit 1 ./tabbed
set the signal handler to SIG_IGN to reap zombies automatically
(according to POSIX.1-2001).
NOTE: this patch follows dwm's commit 712d663. according to the manpage,
none of the sa_flags being set are meaningful since we're using SIG_IGN.
they used to be meaningful on earlier version of the patch where it was
using SIG_DFL, i've kept them here just in case.
---
Makefile | 2 +-
tabbed.c | 21 +
2 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/Makefile b/Makefile
index f8f5ba4..54ba350 100644
--- a/Makefile
+++ b/Makefile
@@ -11,7 +11,7 @@ DOCPREFIX = ${PREFIX}/share/doc/${NAME}
# use system flags.
TABBED_CFLAGS = -I/usr/X11R6/include -I/usr/include/freetype2 ${CFLAGS}
TABBED_LDFLAGS = -L/usr/X11R6/lib -lX11 -lfontconfig -lXft ${LDFLAGS}
-TABBED_CPPFLAGS = -DVERSION=\"${VERSION}\" -D_DEFAULT_SOURCE
+TABBED_CPPFLAGS = -DVERSION=\"${VERSION}\" -D_DEFAULT_SOURCE
-D_XOPEN_SOURCE=700L
# OpenBSD (uncomment)
#TABBED_CFLAGS = -I/usr/X11R6/include -I/usr/X11R6/include/freetype2 ${CFLAGS}
diff --git a/tabbed.c b/tabbed.c
index eafe28a..477a041 100644
--- a/tabbed.c
+++ b/tabbed.c
@@ -125,7 +125,6 @@ static void run(void);
static void sendxembed(int c, long msg, long detail, long d1, long d2);
static void setcmd(int argc, char *argv[], int);
static void setup(void);
-static void sigchld(int unused);
static void spawn(const Arg *arg);
static int textnw(const char *text, unsigned int len);
static void toggle(const Arg *arg);
@@ -976,9 +975,16 @@ setup(void)
XWMHints *wmh;
XClassHint class_hint;
XSizeHints *size_hint;
+ struct sigaction sa;
- /* clean up any zombies immediately */
- sigchld(0);
+ /* do not transform children into zombies when they terminate */
+ sigemptyset(_mask);
+ sa.sa_flags = SA_NOCLDSTOP | SA_NOCLDWAIT | SA_RESTART;
+ sa.sa_handler = SIG_IGN;
+ sigaction(SIGCHLD, , NULL);
+
+ /* clean up any zombies that might have been inherited */
+ while (waitpid(-1, NULL, WNOHANG) > 0);
/* init screen */
screen = DefaultScreen(dpy);
@@ -1078,15 +1084,6 @@ setup(void)
focus(-1);
}
-void
-sigchld(int unused)
-{
- if (signal(SIGCHLD, sigchld) == SIG_ERR)
- die("%s: cannot install SIGCHLD handler", argv0);
-
- while (0 < waitpid(-1, NULL, WNOHANG));
-}
-
void
spawn(const Arg *arg)
{
--
2.39.1
Re: [hackers] [dmenu] readstdin: reduce memory-usage by duplicating the line from getline() || Hiltjo Posthuma
Was wondering what happens if we instead use realloc to shrink the
buffer. On my (glibc v2.36) system, it doesn't seem to make any
difference, though.
index 27b7a30..fc127ae 100644
@@ -562,6 +562,8 @@ readstdin(void)
}
if (line[len - 1] == '\n')
line[len - 1] = '\0';
+ if (!(line = realloc(line, len + 1)))
+ die("realloc:");
items[i].text = line;
items[i].out = 0;
line = NULL; /* next call of getline() allocates a new
line */
Tested with `seq 0 128000 | ./dmenu`:
Old (getline) ~29K
realloc~29K
new (strdup) ~17K
Maybe using `malloc(len + 1) + memcpy` would be better for larger
strings instead of strdup. But hopefully no one is going to be piping
gigabytes long lines into dmenu for it to matter :) So it's fine as is.
- NRK
[hackers] [dwm][PATCH] config.mk: update to _XOPEN_SOURCE=700L
SA_NOCLDWAIT is marked as XSI in the posix spec [0] and freebsd seems to
more be strict about the feature test macro [1].
so update the macro to use _XOPEN_SOURCE=700L instead, which is
equivalent to _POSIX_C_SOURCE=200809L except that it also unlocks the
X/Open System Interfaces.
[0]:
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/signal.h.html#tag_13_42
[1]: https://lists.suckless.org/dev/2302/35111.html
Reported-by: beastie
---
config.mk | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/config.mk b/config.mk
index ef8acf7..ba64d3d 100644
--- a/config.mk
+++ b/config.mk
@@ -26,7 +26,7 @@ INCS = -I${X11INC} -I${FREETYPEINC}
LIBS = -L${X11LIB} -lX11 ${XINERAMALIBS} ${FREETYPELIBS}
# flags
-CPPFLAGS = -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_POSIX_C_SOURCE=200809L
-DVERSION=\"${VERSION}\" ${XINERAMAFLAGS}
+CPPFLAGS = -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE=700L
-DVERSION=\"${VERSION}\" ${XINERAMAFLAGS}
#CFLAGS = -g -std=c99 -pedantic -Wall -O0 ${INCS} ${CPPFLAGS}
CFLAGS = -std=c99 -pedantic -Wall -Wno-deprecated-declarations -Os ${INCS}
${CPPFLAGS}
LDFLAGS = ${LIBS}
--
2.39.1
Re: [hackers] [st][PATCH]] Fixed osc color reset without parameter->resets all colors
On Sun, Feb 05, 2023 at 02:02:47PM +0100, thinkerwim wrote: > just a question for next time. What did you mean by white space garbled.. ST (and most other suckless code) uses TABs for indentation. A common mistake is that people end up using spaces instead of TABs (and cannot tell the difference because their editor isn't configured to distinguish TABs vs spaces). But the patch you sent didn't contain _any_ indentation at all. Which is rather weird. - NRK
Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling
On Sat, Jan 28, 2023 at 12:53:48PM +0100, Hiltjo Posthuma wrote: > waitpid() and sigaction() can also fail with EINTR, which may mean our zombie > handling fails, so block signals while setting things up to be careful. IMO this line should be removed from the commit message because: * It's not accurate (afaik) becasuse EINTR isn't specified for `sigaction`: https://www.man7.org/linux/man-pages/man3/sigaction.3p.html#ERRORS * the signal blocking got removed from the final patch anyways. But otherwise it's looking OK to me :) - NRK
Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling
Hi Hiltjo,
On Sat, Jan 28, 2023 at 12:11:44AM +0100, Hiltjo Posthuma wrote:
> We do not need to waitpid() on child processes. It is handled already
> by using sigaction().
Here's a test case. I see `cat` turning into a zombie on my system
without the `waitpid`:
[/tmp]~> cat test.c
#include
#include
#include
#include
#include
int main(void)
{
struct sigaction sa;
puts("waiting for cat to become a zombie");
sleep(1);
sigemptyset(_mask);
sa.sa_flags = SA_NOCLDSTOP | SA_NOCLDWAIT | SA_RESTART;
sa.sa_handler = SIG_IGN;
sigaction(SIGCHLD, , NULL);
puts("hello there");
(void)getchar();
}
[/tmp]~> cat test.sh
#!/bin/sh
cat &
exec ./test
[/tmp]~> cc -o test test.c
[/tmp]~> ./test.sh
waiting for cat to become a zombie
hello there
Just putting `while (waitpid(-1, NULL, WNOHANG) > 0);` after the
`sigaction` call (without worrying about EINTR) should still be better
than not calling `waitpid` at all IMO.
- NRK
Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling
On Wed, Jan 25, 2023 at 09:17:53PM +0100, Hiltjo Posthuma wrote:
> Using the new patch it does not handle zombie/defunct processes anymore on
> this
> machine. To reproduce it, in .xinitrc, on a dusty machine:
>
> sleep 10 &
> exec dwm
For ease of testing I wrote this dummy program. Both SIG_DFL and SIG_IGN
works on my system (glibc 2.36 & linux 5.15.13).
[/tmp]~> cat test.c
#include
#include
#include
#include
int main(void)
{
const struct sigaction sc = {
.sa_handler = SIG_DFL,
.sa_flags = SA_RESTART | SA_NOCLDWAIT | SA_NOCLDSTOP,
};
if (sigaction(SIGCHLD, , NULL) < 0)
abort();
while (waitpid(-1, NULL, WNOHANG) > 0);
puts("hello there");
(void)getchar();
}
[/tmp]~> cc -o test test.c
[/tmp]~> cat test.sh
#!/bin/sh
sleep 4 &
exec ./test
[/tmp]~> ./test.sh
And the `signal(3p)` version also works:
int main(void)
{
if (signal(SIGCHLD, SIG_IGN) < 0)
abort();
while (waitpid(-1, NULL, WNOHANG) > 0);
puts("hello there");
(void)getchar();
}
> Also, maybe it is better to not call sigprocmask? Since now it blocks all
> signals, including SIGTERM in setup().
@Chris: I'm looking at the POSIX manpage for sigaction and I don't think
it can fail due to interrupt. Only `EINVAL` is specified:
https://www.man7.org/linux/man-pages/man3/sigaction.3p.html#ERRORS
So getting rid of the `sigprocmask` and manually testing for EINTR when
reaping inherited zombies should be fine, I think.
- NRK
Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling
On Wed, Jan 25, 2023 at 09:37:58AM +0100, Hiltjo Posthuma wrote: > I don't think there can be any zombies since it is run in setup() before any > processes can spawn (via keybindings etc) or am I overlooking something > obvious? It was recently discussed here: https://lists.suckless.org/hackers/2207/18424.html And here's the original thread from 2009: https://lists.suckless.org/dev/0908/0774.html - NRK
Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling
On Tue, Jan 24, 2023 at 09:01:08PM +0100, Hiltjo Posthuma wrote:
> Although of course checking errno on a success condition is a bit wonky in
> this test case.
It was just an illustration that the malloc succeeded :)
A more real-world check would be something like this (in fact, I'm quite
sure this is precisely the cause of the xlib XIO error):
if (poll(...) < 0) {
if (errno == EINTR)
continue;
else
error(...);
}
> What about a simplified version of a patch below, it should do a similar
> thing:
>
> /* clean up any zombies immediately */
> - sigchld(0);
> + if (signal(SIGCHLD, SIG_IGN) == SIG_ERR)
> + die("can't ignore SIGCHLD:");
One issue here is that this will not clean up any existing zombies
(inherited from .xinitrc etc).
That's what that "clean up any zombies immediately" comment is referring
to. I think that comment ought to be a bit more descriptive and mention
where these zombies are coming from since it's not immediately obvious.
One other thing worth mentioning is that this behavior wasn't well
defined on earlier POSIX versions so some historical BSDs may have
different behavior. From
(https://man7.org/linux/man-pages/man2/sigaction.2.html#NOTES):
| POSIX.1-1990 disallowed setting the action for SIGCHLD to
| SIG_IGN. POSIX.1-2001 and later allow this possibility, so that
| ignoring SIGCHLD can be used to prevent the creation of zombies.
| Nevertheless, the historical BSD and System V behaviors for ignoring
| SIGCHLD differ
The POSIX manpage for signal(3p) also says that "new applications should
use sigaction() rather than signal()" - probably due to these
incompatibility reasons.
But in any case, if you don't want to use `sigaction`, I think reaping
the existing zombies after the `signal` call _should_ work:
- /* clean up any zombies immediately */
- sigchld(0);
+ if (signal(SIGCHLD, SIG_IGN) == SIG_ERR)
+ die("can't ignore SIGCHLD:");
+ /* clean up any zombies (inherited from .xinitrc etc)
immediately */
+ while (waitpid(-1, NULL, WNOHANG) > 0);
I haven't tested (or even compiled) the above patch. And it seems that
signals (much like threads) are really easy to mess up, so feel free to
correct if anything is wrong.
- NRK
Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling
On Mon, Jan 23, 2023 at 08:05:19PM +0100, Hiltjo Posthuma wrote:
> Do you perhaps also have some simple way to reproduce where it causes issues
> in
> some real world use-case?
>
> Ideally some command or script to reproduce the issue.
To trigger the issue, you need to have 3 conditions met:
1. Recieve a SIGCHLD.
2. `waitpid` failing and spoiling the `errno`.
3. Being unlucky that the resuming code gets affected by the `errno`.
Here's a simple dummy program demonstrating the issue. Compile it, run
it and then send some SIGCHLDs to it:
[/tmp]~> cat sigtest.c
#include
#include
#include
#include
#include
void
sigchld(int unused)
{
if (signal(SIGCHLD, sigchld) == SIG_ERR)
_Exit(1);
while (0 < waitpid(-1, NULL, WNOHANG));
}
int main(void)
{
sigchld(0);
while (1) {
errno = 0;
char *p = malloc(8);
if (p != NULL && errno) {
perror("sigtest: ");
return 1;
}
free(p);
}
}
[/tmp]~> cc -o sigtest sigtest.c
[/tmp]~> ./sigtest &
[1] 9363
[/tmp]~> while pidof sigtest >/dev/null; do kill -s SIGCHLD $(pidof
sigtest); done
sigtest: : No child processes
If you're asking for `dwm` in specific - then trigger condition 2 is
difficult (impossible ??) since the window manager will typically have
some children. But if you insert the following, to simulate failure:
while (0 < waitpid(-1, NULL, WNOHANG));
+ errno = ECHILD;
then the issue is easily reproducable by sending some SIGCHLDs to dwm
(in quick succession in order to maximize chances of triggering
condition 3):
var=$(pidof dwm); while :; do kill -s SIGCHLD $var; done
And dwm will crash with a XIO error with in a couple seconds on my
system.
- NRK
Re: [hackers] [dwm 6.4][movewithtag]
On Mon, Jan 23, 2023 at 08:56:13AM -0300, Dr. André Desgualdo Pereira wrote: > I'm submitting a new patch to be appreciated and included on suckless site You can push patches to the site directly yourself. See https://suckless.org/wiki/ - NRK
[hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling
On Wed, Jul 27, 2022 at 07:36:28AM +0100, Chris Down wrote:
> -void
> -sigchld(int unused)
> -{
> - if (signal(SIGCHLD, sigchld) == SIG_ERR)
> - die("can't install SIGCHLD handler:");
> - while (0 < waitpid(-1, NULL, WNOHANG));
> -}
One other problem with this type of sighandler that I recently realized
(thanks to Thread-Sanitizer [0]) is that the `waitpid` call may end up
spoiling the `errno`. So when execution resumes, the resuming code can
end up behaving incorrectly due to that.
And as demonstrated in [0] this can occur in practice and is not just a
theoretical issue. Granted that it's much less likely to occur in dwm
since spawning children is done rarely (compared to nsxiv where there
might be upto 2 processes spawned at each image load) but the fact that
crashes like this is possible isn't great to begin with.
If there's any specific issue with Chris' patch which is blocking the
merge then I'd be happy to address them.
[0]: https://codeberg.org/nsxiv/nsxiv/issues/391#issuecomment-776947
- NRK
Re: [hackers] [slstatus] More LICENSE updates || drkhsh
On Mon, Dec 19, 2022 at 10:04:56PM +0100, g...@suckless.org wrote: > -Copyright 2022 NRK > +Copyright 2022 Nickolas Raymond Kaczynski Would prefer just NRK :) And as far as I'm aware, using pseudonames for copyright should be fine. - NRK
Re: [hackers] [libgrapheme] Do not falsely read entire buffer instead of simply the filled with || Laslo Hunhold
> This was caught via static analysis (clang asan), which I can definitely > recommend. Small nitpick: ASan (and the other sanitizers) are *dynamic* analyzers, as they happen during runtime. Static analysis is analyzing without executing anything. Examples of static analyzers would be clang-tidy or cppcheck. Newer GCC versions also have a `-fanalyzer` flag for statically analyzing C code, but in my experience it's not mature yet - but the direction looks promising. - NRK
[hackers] [dmenu][PATCH] fix leak when getline fails
according to the getline(3) documentation, the calling code needs to
free the buffer even if getline fails.
dmenu currently doesn't do that which results in a small leak in case of
failure (e.g when piped /dev/null)
$ ./dmenu < /dev/null
==8201==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 120 byte(s) in 1 object(s) allocated from:
#0 0x7f6bf5785ef7 in malloc
#1 0x7f6bf538ec84 in __getdelim
#2 0x405d0c in readstdin dmenu.c:557
moving `line = NULL` inside the loop body wasn't strictly necessary, but
IMO it makes it more apparent that `line` is getting cleared to NULL
after each successful iteration.
---
dmenu.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/dmenu.c b/dmenu.c
index e7be8af..e786d7a 100644
--- a/dmenu.c
+++ b/dmenu.c
@@ -554,7 +554,7 @@ readstdin(void)
ssize_t len;
/* read each line from stdin and add it to the item list */
- for (i = 0; (len = getline(, , stdin)) != -1; i++, line =
NULL) {
+ for (i = 0; (len = getline(, , stdin)) != -1; i++) {
if (i + 1 >= size / sizeof *items)
if (!(items = realloc(items, (size += BUFSIZ
die("cannot realloc %zu bytes:", size);
@@ -562,7 +562,9 @@ readstdin(void)
line[len - 1] = '\0';
items[i].text = line;
items[i].out = 0;
+ line = NULL;
}
+ free(line);
if (items)
items[i].text = NULL;
lines = MIN(lines, i);
--
2.35.1
[hackers] Re: [dwm][PATCH] applyrules: fix potential false positive match
On Sat, Oct 29, 2022 at 04:59:29PM +0600, NRK wrote: > - class= ch.res_class ? ch.res_class : broken; > - instance = ch.res_name ? ch.res_name : broken; > + class= ch.res_class ? ch.res_class : ""; > + instance = ch.res_name ? ch.res_name : ""; After some discussion on IRC and going through `git blame` (see commit 8dc9fcf) it seems like this behavior is intended (though questionable). In any case, feel free to ignore this. - NRK
[hackers] [dwm][PATCH] applyrules: fix potential false positive match
if res_{class,name} is null, it is set to broken (which is just
"broken") to avoid calling strstr() with NULL.
but doing this can end up having false positive matches. so set them to
an empty string instead of "broken".
---
dwm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/dwm.c b/dwm.c
index e5efb6a..0e32987 100644
--- a/dwm.c
+++ b/dwm.c
@@ -289,8 +289,8 @@ applyrules(Client *c)
c->isfloating = 0;
c->tags = 0;
XGetClassHint(dpy, c->win, );
- class= ch.res_class ? ch.res_class : broken;
- instance = ch.res_name ? ch.res_name : broken;
+ class= ch.res_class ? ch.res_class : "";
+ instance = ch.res_name ? ch.res_name : "";
for (i = 0; i < LENGTH(rules); i++) {
r = [i];
--
2.35.1
Re: [hackers] [slstatus][PATCH] do not rely on obsolete feature
On Wed, Oct 26, 2022 at 09:37:28PM +, drkhsh wrote:
> Totally agree that this is more correct than the way it was. Thanks. But
> maybe we can find some even cleaner solution for the upcoming 1.0 release.
In dwm it's done through a union:
typedef union {
int i;
unsigned int ui;
float f;
const void *v;
} Arg;
I was thinking of doing something similar but then I noticed that
slstatus doesn't pass any integer or floats, only strings. So I went
with `const char *`.
If there's plans of having functions which might accept integers/floats
or any other types in the future then using a union similar to dwm might
be worth it.
- NRK
[hackers] [slstatus][PATCH] do not rely on obsolete feature
function prototype with unspecified argument is obsolete since c99.
additionally some of these function which don't take any argument were
being called with a `const char *` arg, which is UB.
fix both these issues by declararing ALL the components to accept a
`const char *`, and name the arg "unused" if it's meant to be ignored.
---
components/cpu.c| 12 ++--
components/entropy.c| 4 ++--
components/hostname.c | 2 +-
components/kernel_release.c | 2 +-
components/keymap.c | 2 +-
components/load_avg.c | 2 +-
components/ram.c| 24 +++
components/swap.c | 24 +++
components/uptime.c | 2 +-
components/user.c | 6 +++---
config.mk | 2 +-
slstatus.c | 2 +-
slstatus.h | 38 ++---
13 files changed, 61 insertions(+), 61 deletions(-)
diff --git a/components/cpu.c b/components/cpu.c
index a5fabf8..254f047 100644
--- a/components/cpu.c
+++ b/components/cpu.c
@@ -8,7 +8,7 @@
#if defined(__linux__)
const char *
- cpu_freq(void)
+ cpu_freq(const char *unused)
{
uintmax_t freq;
@@ -22,7 +22,7 @@
}
const char *
- cpu_perc(void)
+ cpu_perc(const char *unused)
{
static long double a[7];
long double b[7], sum;
@@ -55,7 +55,7 @@
#include
const char *
- cpu_freq(void)
+ cpu_freq(const char *unused)
{
int freq, mib[2];
size_t size;
@@ -75,7 +75,7 @@
}
const char *
- cpu_perc(void)
+ cpu_perc(const char *unused)
{
int mib[2];
static uintmax_t a[CPUSTATES];
@@ -115,7 +115,7 @@
#include
const char *
- cpu_freq(void)
+ cpu_freq(const char *unused)
{
int freq;
size_t size;
@@ -132,7 +132,7 @@
}
const char *
- cpu_perc(void)
+ cpu_perc(const char *unused)
{
size_t size;
static long a[CPUSTATES];
diff --git a/components/entropy.c b/components/entropy.c
index cf60bc3..0544749 100644
--- a/components/entropy.c
+++ b/components/entropy.c
@@ -7,7 +7,7 @@
#include "../util.h"
const char *
- entropy(void)
+ entropy(const char *unused)
{
uintmax_t num;
@@ -20,7 +20,7 @@
}
#elif defined(__OpenBSD__) | defined(__FreeBSD__)
const char *
- entropy(void)
+ entropy(const char *unused)
{
/* Unicode Character 'INFINITY' (U+221E) */
return "\xe2\x88\x9e";
diff --git a/components/hostname.c b/components/hostname.c
index 8627a88..f04a913 100644
--- a/components/hostname.c
+++ b/components/hostname.c
@@ -6,7 +6,7 @@
#include "../slstatus.h"
const char *
-hostname(void)
+hostname(const char *unused)
{
if (gethostname(buf, sizeof(buf)) < 0) {
warn("gethostbyname:");
diff --git a/components/kernel_release.c b/components/kernel_release.c
index 76a3827..e21fefe 100644
--- a/components/kernel_release.c
+++ b/components/kernel_release.c
@@ -6,7 +6,7 @@
#include "../slstatus.h"
const char *
-kernel_release(void)
+kernel_release(const char *unused)
{
struct utsname udata;
diff --git a/components/keymap.c b/components/keymap.c
index 87036db..2a99474 100644
--- a/components/keymap.c
+++ b/components/keymap.c
@@ -47,7 +47,7 @@ get_layout(char *syms, int grp_num)
}
const char *
-keymap(void)
+keymap(const char *unused)
{
Display *dpy;
XkbDescRec *desc;
diff --git a/components/load_avg.c b/components/load_avg.c
index 2b523e7..ab33fa3 100644
--- a/components/load_avg.c
+++ b/components/load_avg.c
@@ -6,7 +6,7 @@
#include "../slstatus.h"
const char *
-load_avg(void)
+load_avg(const char *unused)
{
double avgs[3];
diff --git a/components/ram.c b/components/ram.c
index 3fa78af..d90b107 100644
--- a/components/ram.c
+++ b/components/ram.c
@@ -8,7 +8,7 @@
#include
const char *
- ram_free(void)
+ ram_free(const char *unused)
{
uintmax_t free;
@@ -24,7 +24,7 @@
}
const char *
- ram_perc(void)
+ ram_perc(const char *unused)
{
uintmax_t total, free, buffers, cached;
@@ -47,7 +47,7 @@
}
const char *
- ram_total(void)
+ ram_total(const char *unused)
{
uintmax_t total;
@@ -60,7 +60,7 @@
}
const char *
- ram_used(void)
+ ram_used(const char *unused)
{
uintmax_t total, free, buffers, cached;
@@ -102,7 +102,7 @@
}
const char *
- ram_free(void)
+ ram_free(const char *unused)
{
struct
[hackers] [slstatus][PATCH] components/*.c: include slstatus.h
this gives the compiler a chance to check weather the prototype and definiton matches or not, which would catch issues like 3c47701. --- Makefile | 2 +- components/battery.c | 1 + components/cpu.c | 1 + components/datetime.c| 1 + components/disk.c| 1 + components/entropy.c | 1 + components/hostname.c| 1 + components/ip.c | 1 + components/kernel_release.c | 1 + components/keyboard_indicators.c | 1 + components/keymap.c | 1 + components/load_avg.c| 1 + components/netspeeds.c | 1 + components/num_files.c | 1 + components/ram.c | 1 + components/run_command.c | 1 + components/separator.c | 1 + components/swap.c| 1 + components/temperature.c | 1 + components/uptime.c | 1 + components/user.c| 1 + components/volume.c | 1 + components/wifi.c| 1 + 23 files changed, 23 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 2f93b87..542b1f0 100644 --- a/Makefile +++ b/Makefile @@ -31,7 +31,7 @@ COM =\ all: slstatus -$(COM:=.o): config.mk $(REQ:=.h) +$(COM:=.o): config.mk $(REQ:=.h) slstatus.h slstatus.o: slstatus.c slstatus.h arg.h config.h config.mk $(REQ:=.h) .c.o: diff --git a/components/battery.c b/components/battery.c index a36132d..b7a2ec6 100644 --- a/components/battery.c +++ b/components/battery.c @@ -3,6 +3,7 @@ #include #include "../util.h" +#include "../slstatus.h" #if defined(__linux__) #include diff --git a/components/cpu.c b/components/cpu.c index 9e28003..a5fabf8 100644 --- a/components/cpu.c +++ b/components/cpu.c @@ -4,6 +4,7 @@ #include #include "../util.h" +#include "../slstatus.h" #if defined(__linux__) const char * diff --git a/components/datetime.c b/components/datetime.c index c3efae3..cf9fbba 100644 --- a/components/datetime.c +++ b/components/datetime.c @@ -3,6 +3,7 @@ #include #include "../util.h" +#include "../slstatus.h" const char * datetime(const char *fmt) diff --git a/components/disk.c b/components/disk.c index 15a221b..64e2105 100644 --- a/components/disk.c +++ b/components/disk.c @@ -3,6 +3,7 @@ #include #include "../util.h" +#include "../slstatus.h" const char * disk_free(const char *path) diff --git a/components/entropy.c b/components/entropy.c index 2a485de..cf60bc3 100644 --- a/components/entropy.c +++ b/components/entropy.c @@ -1,4 +1,5 @@ /* See LICENSE file for copyright and license details. */ +#include "../slstatus.h" #if defined(__linux__) #include #include diff --git a/components/hostname.c b/components/hostname.c index 23da677..8627a88 100644 --- a/components/hostname.c +++ b/components/hostname.c @@ -3,6 +3,7 @@ #include #include "../util.h" +#include "../slstatus.h" const char * hostname(void) diff --git a/components/ip.c b/components/ip.c index 70724eb..6d1da68 100644 --- a/components/ip.c +++ b/components/ip.c @@ -12,6 +12,7 @@ #endif #include "../util.h" +#include "../slstatus.h" static const char * ip(const char *interface, unsigned short sa_family) diff --git a/components/kernel_release.c b/components/kernel_release.c index 0457301..76a3827 100644 --- a/components/kernel_release.c +++ b/components/kernel_release.c @@ -3,6 +3,7 @@ #include #include "../util.h" +#include "../slstatus.h" const char * kernel_release(void) diff --git a/components/keyboard_indicators.c b/components/keyboard_indicators.c index b35eba1..5f482e8 100644 --- a/components/keyboard_indicators.c +++ b/components/keyboard_indicators.c @@ -5,6 +5,7 @@ #include #include "../util.h" +#include "../slstatus.h" /* * fmt consists of uppercase or lowercase 'c' for caps lock and/or 'n' for num diff --git a/components/keymap.c b/components/keymap.c index ddf7a15..87036db 100644 --- a/components/keymap.c +++ b/components/keymap.c @@ -6,6 +6,7 @@ #include #include "../util.h" +#include "../slstatus.h" static int valid_layout_or_variant(char *sym) diff --git a/components/load_avg.c b/components/load_avg.c index 5c2e252..2b523e7 100644 --- a/components/load_avg.c +++ b/components/load_avg.c @@ -3,6 +3,7 @@ #include #include "../util.h" +#include "../slstatus.h" const char * load_avg(void) diff --git a/components/netspeeds.c b/components/netspeeds.c index 0029177..f7bac2e 100644 --- a/components/netspeeds.c +++ b/components/netspeeds.c @@ -3,6 +3,7 @@ #include #include "../util.h" +#include "../slstatus.h" #if defined(__linux__) #include diff --git a/components/num_files.c b/components/num_files.c index fb55df9..3911da7 100644 --- a/components/num_files.c +++ b/components/num_files.c @@ -4,6 +4,7 @@ #include #include "../util.h" +#include "../slstatus.h" const char * num_files(const char *path) diff --git a/components/ram.c
Re: [hackers] [st][PATCH] Fix buffer overflow when handling composed input
On Mon, Oct 24, 2022 at 01:10:29PM +0300, Santtu Lakkala wrote:
> The dynmaic[sic] version incorrectly passes sizeof(buf), where buf is char
> *, as the size of buffer in the "happy case" leading to unnecessary hits to
> the dynamic path.
Ah yes, the classic. Attached ammended version of the dynmaic patch.
A bit more invasive (but hopefully correct) than the previous patch.
- NRK
>From 920799cb2a3526f2398caf98ebf0815fb323ff3f Mon Sep 17 00:00:00 2001
From: NRK
Date: Mon, 24 Oct 2022 15:49:18 +0600
Subject: [PATCH] fix: buffer overflow when handling composed input
XmbLookupString may leave buf as well as ksym uninitialized. initialize
ksym to NoSymbol. track weather we need a larger buffer or not, and
dynamically allocate if needed.
Reported-by: Andy Gozas
---
x.c | 30 --
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/x.c b/x.c
index f70e3fb..6a4ed46 100644
--- a/x.c
+++ b/x.c
@@ -1847,8 +1847,8 @@ kpress(XEvent *ev)
{
XKeyEvent *e = >xkey;
KeySym ksym;
- char buf[64], *customkey;
- int len;
+ char buf[64], *p = NULL, *customkey;
+ int len, overflow = 0;
Rune c;
Status status;
const Shortcut *bp;
@@ -1856,10 +1856,12 @@ kpress(XEvent *ev)
if (IS_SET(MODE_KBDLOCK))
return;
- if (xw.ime.xic)
+ if (xw.ime.xic) {
len = XmbLookupString(xw.ime.xic, e, buf, sizeof buf, , );
- else
+ overflow = status == XBufferOverflow ? len : 0;
+ } else {
len = XLookupString(e, buf, sizeof buf, , NULL);
+ }
/* 1. shortcuts */
for (bp = shortcuts; bp < shortcuts + LEN(shortcuts); bp++) {
if (ksym == bp->keysym && match(bp->mod, e->state)) {
@@ -1875,21 +1877,29 @@ kpress(XEvent *ev)
}
/* 3. composed string from input method */
+ if (overflow) {
+ p = xmalloc(overflow);
+ len = XmbLookupString(xw.ime.xic, e, p, overflow, , );
+ } else {
+ p = buf;
+ }
if (len == 0)
return;
if (len == 1 && e->state & Mod1Mask) {
if (IS_SET(MODE_8BIT)) {
- if (*buf < 0177) {
-c = *buf | 0x80;
-len = utf8encode(c, buf);
+ if (*p < 0177) {
+c = *p | 0x80;
+len = utf8encode(c, p);
}
} else {
- buf[1] = buf[0];
- buf[0] = '\033';
+ p[1] = p[0];
+ p[0] = '\033';
len = 2;
}
}
- ttywrite(buf, len, 1);
+ ttywrite(p, len, 1);
+ if (overflow)
+ free(p);
}
void
--
2.35.1
Re: [hackers] [st][PATCH] Fix buffer overflow when handling composed input
On Mon, Oct 24, 2022 at 09:21:37AM +, Andy Gozas wrote:
> • XmbLookupString leaves the ksym unchanged if not filled and XLookupString
> [probably] sets it to NoSymbol (that's what XLookupKeysym does, but whether
> or not XLookupString shares this behavior is unclear [1]), so we can just
> set it to NoSymbol in the beginning ourselves and check if it was changed
> later
Initializing ksym to `NoSymbol` seems like a good idea.
> • Since we can actually get the whole composed text when using
> XmbLookupString by reallocating the buffer, I think we should do that — why
> stop at 512 bytes?
Mainly because using I think that the dynamic allocation patch made the
control flow of the function more complicated than necessary.
Backward goto is pretty bad in specific.
But if you _do_ want to dynamically allocate, you only need to allocate
right before buffer is being used.
But which approach to take is the maintainer's call, not mine.
I've attched both fixed-size and dynamic-allocation patch (but
simplified without goto).
- NRK
>From 6eae20945177667a0331670d0ca1c75226cfd29a Mon Sep 17 00:00:00 2001
From: NRK
Date: Mon, 24 Oct 2022 15:35:17 +0600
Subject: [PATCH] fix: buffer overflow when handling composed input
XmbLookupString may leave buf as well as ksym uninitialized. initialize
ksym to NoSymbol and track weather buffer was initialized or not via
`got_buf`.
Reported-by: Andy Gozas
---
x.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/x.c b/x.c
index f70e3fb..b344dad 100644
--- a/x.c
+++ b/x.c
@@ -1846,20 +1846,24 @@ void
kpress(XEvent *ev)
{
XKeyEvent *e = >xkey;
- KeySym ksym;
- char buf[64], *customkey;
+ KeySym ksym = NoSymbol;
+ char buf[512], *customkey;
int len;
Rune c;
Status status;
const Shortcut *bp;
+ int got_buf = 0;
if (IS_SET(MODE_KBDLOCK))
return;
- if (xw.ime.xic)
+ if (xw.ime.xic) {
len = XmbLookupString(xw.ime.xic, e, buf, sizeof buf, , );
- else
+ got_buf = status == XLookupBoth || status == XLookupChars;
+ } else {
len = XLookupString(e, buf, sizeof buf, , NULL);
+ got_buf = 1;
+ }
/* 1. shortcuts */
for (bp = shortcuts; bp < shortcuts + LEN(shortcuts); bp++) {
if (ksym == bp->keysym && match(bp->mod, e->state)) {
@@ -1875,7 +1879,7 @@ kpress(XEvent *ev)
}
/* 3. composed string from input method */
- if (len == 0)
+ if (len == 0 || !got_buf)
return;
if (len == 1 && e->state & Mod1Mask) {
if (IS_SET(MODE_8BIT)) {
--
2.35.1
>From ea78eacf892ee232a5a060fa1204f896841f7ec5 Mon Sep 17 00:00:00 2001
From: NRK
Date: Mon, 24 Oct 2022 15:49:18 +0600
Subject: [PATCH] fix: buffer overflow when handling composed input
XmbLookupString may leave buf as well as ksym uninitialized. initialize
ksym to NoSymbol. track weather we need a larger buffer or not, and
dynamically allocate if needed.
Reported-by: Andy Gozas
---
x.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/x.c b/x.c
index f70e3fb..e50ac90 100644
--- a/x.c
+++ b/x.c
@@ -1847,8 +1847,8 @@ kpress(XEvent *ev)
{
XKeyEvent *e = >xkey;
KeySym ksym;
- char buf[64], *customkey;
- int len;
+ char small_buf[512], *buf = small_buf, *customkey;
+ int len, buf_overflow = 0;
Rune c;
Status status;
const Shortcut *bp;
@@ -1856,10 +1856,12 @@ kpress(XEvent *ev)
if (IS_SET(MODE_KBDLOCK))
return;
- if (xw.ime.xic)
+ if (xw.ime.xic) {
len = XmbLookupString(xw.ime.xic, e, buf, sizeof buf, , );
- else
+ buf_overflow = status == XBufferOverflow ? len : 0;
+ } else {
len = XLookupString(e, buf, sizeof buf, , NULL);
+ }
/* 1. shortcuts */
for (bp = shortcuts; bp < shortcuts + LEN(shortcuts); bp++) {
if (ksym == bp->keysym && match(bp->mod, e->state)) {
@@ -1875,6 +1877,10 @@ kpress(XEvent *ev)
}
/* 3. composed string from input method */
+ if (buf_overflow) {
+ buf = xmalloc(buf_overflow);
+ len = XmbLookupString(xw.ime.xic, e, buf, buf_overflow, , );
+ }
if (len == 0)
return;
if (len == 1 && e->state & Mod1Mask) {
@@ -1890,6 +1896,8 @@ kpress(XEvent *ev)
}
}
ttywrite(buf, len, 1);
+ if (buf_overflow)
+ free(buf);
}
void
--
2.35.1
Re: [hackers] [surf][PATCH 2/2] webext: use flags for gio-unix rather than gio
Hi Quentin, > On my system, gio-unix symbols are provided by the gio library, > is that different on your system? > If so, could you provide the contents of pkg-config cflags and libs > for gio-unix-2.0? In the "old documentation" [0] I see the following: | Note that belongs to the UNIX-specific GIO | interfaces, thus you have to use the gio-unix-2.0.pc pkg-config file | when using it. And in the (hopefully current) docs [1]: | Before 2.74, belonged to the UNIX-specific GIO | interfaces, thus you had to use the gio-unix-2.0.pc pkg-config file | when using it. | | Since 2.74, the API is available for Windows. It seems that using `gio-unix-2.0` explicitly should be more portable. [0]: https://developer-old.gnome.org/gio/stable/GUnixFDList.html [1]: https://docs.gtk.org/gio/class.UnixFDList.html - NRK
Re: [hackers] [st][PATCH] Fix buffer overflow when handling composed input
> On Sun, Oct 23, 2022 at 04:18:42PM +, Andy Gozas wrote:
> > St relies on an incorrect assumption of how XmbLookupString function
> > behaves.
Looking at the XmbLookupString manpage [0] reveals more trouble. It seems
that `ksym` might be used uninitalized as well. Inlined a proprosed
patch.
P.S: Please CC me on any replies, I seem to be missing a lot of mails
from the ML recently.
[0]: https://www.x.org/releases/X11R7.5/doc/man/man3/Xutf8LookupString.3.html
- NRK
diff --git a/x.c b/x.c
index f70e3fb..63886c7 100644
--- a/x.c
+++ b/x.c
@@ -1847,35 +1847,40 @@ kpress(XEvent *ev)
{
XKeyEvent *e = >xkey;
KeySym ksym;
- char buf[64], *customkey;
+ char buf[512], *customkey;
int len;
Rune c;
Status status;
const Shortcut *bp;
+ int got_buf = 0, got_ksym = 0;
if (IS_SET(MODE_KBDLOCK))
return;
- if (xw.ime.xic)
+ if (xw.ime.xic) {
len = XmbLookupString(xw.ime.xic, e, buf, sizeof buf, ,
);
- else
+ got_buf = status == XLookupBoth || status == XLookupChars;
+ got_ksym = status == XLookupBoth || status == XLookupKeySym;
+ } else {
len = XLookupString(e, buf, sizeof buf, , NULL);
+ got_buf = got_ksym = 1; /* TODO: is this correct? */
+ }
/* 1. shortcuts */
for (bp = shortcuts; bp < shortcuts + LEN(shortcuts); bp++) {
- if (ksym == bp->keysym && match(bp->mod, e->state)) {
+ if (got_ksym && ksym == bp->keysym && match(bp->mod, e->state))
{
bp->func(&(bp->arg));
return;
}
}
/* 2. custom keys from config.h */
- if ((customkey = kmap(ksym, e->state))) {
+ if (got_ksym && (customkey = kmap(ksym, e->state))) {
ttywrite(customkey, strlen(customkey), 1);
return;
}
/* 3. composed string from input method */
- if (len == 0)
+ if (len == 0 || !got_buf)
return;
if (len == 1 && e->state & Mod1Mask) {
if (IS_SET(MODE_8BIT)) {
Re: [hackers] [tabbed] Makefile improvements || Hiltjo Posthuma
On Sun, Oct 16, 2022 at 01:56:51PM +0200, Hiltjo Posthuma wrote:
> > How can it be simpler than calling
> >
> > make CFLAGS=... LDFLAGS=... CPPFLAGS=...
> >
> > ? No matter what you put into config.mk, explicitly specified variables
> > in the make command-line override it.
> >
>
> This includes -DVERSION for example and you'd have to specify all flags,
> including the paths for Freetype of X11 for example. Now it allows both
> options
> more easily, respecting the default CFLAGS, LDFLAGS from the (build) system,
> or
> overriding them completely with TABBED_CFLAGS, TABBED_LDFLAGS.
>From a packagers' perspective, builds that respect environmental
${C,LD}FLAGS are a huge plus for me.
Overriding them via cli is not a good idea since many suckless Makefiles
contain things that are essential for the build in CFLAGS. For example,
this is dwm:
[dwm upstream]~> make CFLAGS="-O3 -march=native"
/usr/include/X11/Xft/Xft.h:40:10: fatal error: ft2build.h: No such file
or directory
40 | #include
| ^~~~
It fails because CPPFLAGS (which contains the -I flags as well as some
feature-test-macros) get mushed with CFLAGS. Dmenu Makefile does the
same thing :|
One other annoyance is that library flags (-l) are typically set in
LDFLAGS in most suckless Makefiles (whereas the convention is to set
them in LDLIBS). This makes overriding LDFLAGS break the build as well:
[dwm upstream]~> make LDFLAGS="-s"
/* thousands of lines of undefined reference linker error */
I earlier said to keep the config.mk, but after looking at it a bit
more, tabbed doesn't really have any build time option (unlike dwm or
dmenu which have optional xcinerama support) so removing `config.mk` I
think is fine since environmental {C,LD}FLAGS are now respected.
- NRK
Re: [hackers] [tabbed] Makefile improvements || Hiltjo Posthuma
> * Remove config.mk This... > diff --git a/config.mk b/config.mk > index e69209e..0c06c2c 100644 > --- a/config.mk > +++ b/config.mk ...does not look "removed" to me :) Also I think it's best to keep it, to be consistent with other suckless projects. - NRK
Re: [hackers] [tabbed] Makefile: add xembed.1 in the dist target || Hiltjo Posthuma
On Wed, Oct 12, 2022 at 11:02:14PM +0200, g...@suckless.org wrote:
> commit 910e67db33dc295b73c1861a79d520b0bd527b2d
> Author: Hiltjo Posthuma
> AuthorDate: Wed Oct 12 22:55:21 2022 +0200
> Commit: Hiltjo Posthuma
> CommitDate: Wed Oct 12 22:55:21 2022 +0200
>
> Makefile: add xembed.1 in the dist target
>
> diff --git a/Makefile b/Makefile
> index 1b95d15..5c8c19e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -37,7 +37,7 @@ dist: clean
> @echo creating dist tarball
> @mkdir -p tabbed-${VERSION}
> @cp -R LICENSE Makefile README config.def.h config.mk \
> - tabbed.1 arg.h ${SRC} tabbed-${VERSION}
> + tabbed.1 xembed.1 arg.h ${SRC} tabbed-${VERSION}
> @tar -cf tabbed-${VERSION}.tar tabbed-${VERSION}
> @gzip tabbed-${VERSION}.tar
> @rm -rf tabbed-${VERSION}
>
Might be worth it to use something like `git ls-files` to grab the list
of files.
- NRK
Re: [hackers] [lchat] minor cleanup and adjustments || Tom Schwindl
> @@ -48,6 +48,14 @@ sigwinch(int sig)
> static void
> exit_handler(void)
> {
> + char *title = getenv("TERM");
> +
> + /* reset terminal's window name */
> + if (strncmp(title, "screen", 6) == 0)
> + printf("\033k%s\033\\", title);
I'd personally test for NULL before passing it to strncmp() just in
case.
> - if (strcmp(getenv("TERM"), "screen") == 0)
> + if (strncmp(getenv("TERM"), "screen", 6) == 0)
> printf("\033k%s\033\\", title);
Same here.
- NRK
Re: [hackers] [libgrapheme] Switch to semantic versioning and improve dynamic library handling || Laslo Hunhold
On Wed, Oct 05, 2022 at 10:51:54PM +0200, g...@suckless.org wrote: > diff --git a/config.mk b/config.mk > index 14aedd3..f8bd40f 100644 > --- a/config.mk > +++ b/config.mk > @@ -1,3 +1,10 @@ > +# libgrapheme version > +VERSION_MAJOR = 1 > +VERSION_MINOR = 0 > +VERSION_PATCH = 0 > +UNICODE_VERSION = 15.0.0 > +MAN_DATE = 2022-09-07 > + Curious, what makes you change you mind about putting these back in config.mk instead of keeping them in the Makefile ? Since they aren't meant to be changed by the user. - NRK
Re: [hackers] [libgrapheme] Check for empty destination before NUL-terminating || Christopher Wellons
On Fri, Oct 07, 2022 at 06:00:30PM +0200, g...@suckless.org wrote:
> commit ad4877023146953d4daa8d91c119124c38620337
> Author: Christopher Wellons
> AuthorDate: Fri Oct 7 11:33:10 2022 -0400
> Commit: Laslo Hunhold
> CommitDate: Fri Oct 7 18:00:11 2022 +0200
>
> Check for empty destination before NUL-terminating
>
> This overflow was triggered in the second test of to_lowercase_utf8
> where the destination is zero length (w->destlen == 0). `w->destlen`
> would overflow by subtraction, then the subscript would overflow the
> destination.
>
> Signed-off-by: Laslo Hunhold
>
> diff --git a/src/util.c b/src/util.c
> index ed893ba..2ff1b64 100644
> --- a/src/util.c
> +++ b/src/util.c
> @@ -207,7 +207,7 @@ herodotus_writer_nul_terminate(HERODOTUS_WRITER *w)
> } else { /* w->type == HERODOTUS_TYPE_UTF8 */
> ((char *)(w->dest))[w->first_unwritable_offset] = '\0';
> }
> - } else {
> + } else if (w->destlen > 0) {
> /*
>* In this case, there is no more space in the buffer and
>* the last unwritable offset is larger than
>
I suppose this makes a good argument for wanting to run the tests under
ASan (and UBsan), at least during local development.
One trick I picked up is that `makefile` has higher precedence than
`Makefile`, which means I can do something like the following:
[libgrapheme]~> git checkout 2.0.0
HEAD is now at ef608a2 Bump to version 2.0.0
[libgrapheme]~> make -s clean
[libgrapheme]~> cat makefile
include Makefile
CFLAGS = -O3 -flto=auto -Wall -Wextra -fsanitize=address,undefined -g
LDFLAGS = $(CFLAGS)
[libgrapheme]~> ASAN_OPTIONS=detect_leaks=0 make -j12 -s test
=
==7679==ERROR: AddressSanitizer: stack-buffer-overflow on address
0x7fffd7f302af at pc 0x00402f50 bp 0x7fffd7f2f990 sp 0x7fffd7f2f988
WRITE of size 1 at 0x7fffd7f302af thread T0
#0 0x402f4f in herodotus_writer_nul_terminate src/util.c:222
#1 0x40e0bd in to_case src/case.c:142
#2 0x40e0bd in grapheme_to_lowercase_utf8 src/case.c:275
#3 0x40e0bd in unit_test_callback_to_case_utf8 test/case.c:529
#4 0x4112a8 in run_unit_tests test/util.c:48
#5 0x401527 in main test/case.c:574
And voila, the overrun gets detected without introducing sanitizer flags
to the Makefile or having to manually specify those flags every time.
A couple notes: `ASAN_OPTIONS=detect_leaks=0` is there to disable the
leak cheker because the LUT generators seems to be leaking memory away.
`-O3 -flto` are there because higher level optimization can also turn on
many warnings which otherwise don't get turned on; LTO in specific can
catch things like parameter type mismatch across TUs which can be rather
difficult to catch otherwise.
- NRK
[hackers] [tabbed][PATCH] don't assume non-null argv[0]
---
xembed.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/xembed.c b/xembed.c
index cbb0e97..0066d04 100644
--- a/xembed.c
+++ b/xembed.c
@@ -15,7 +15,8 @@ main(int argc, char *argv[])
pid_t pgrp, tcpgrp;
if (argc < 3) {
- fprintf(stderr, "usage: %s flag cmd ...\n", argv[0]);
+ fprintf(stderr, "usage: %s flag cmd ...\n",
+ argc > 0 ? argv[0] : "xembed");
return 2;
}
--
2.35.1
[hackers] [tabbed][PATCH] config.h: mark keys as const
---
config.def.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/config.def.h b/config.def.h
index defa426..51bb13d 100644
--- a/config.def.h
+++ b/config.def.h
@@ -34,7 +34,7 @@ static Bool npisrelative = False;
}
#define MODKEY ControlMask
-static Key keys[] = {
+static const Key keys[] = {
/* modifier keyfunction argument */
{ MODKEY|ShiftMask, XK_Return, focusonce, { 0 } },
{ MODKEY|ShiftMask, XK_Return, spawn, { 0 } },
--
2.35.1
Re: [hackers] [libgrapheme] Convert GRAPHEME_STATE to uint_least16_t and remove it || Laslo Hunhold
On Mon, Oct 03, 2022 at 11:28:49PM +0200, g...@suckless.org wrote:
> The expression
>
> GRAPHEME_STATE state = 0;
>
> admittedly looks a little fishy, given you don't really know what happens
> behind the scenes unless you look in the header,
Another possibility is wrapping the integer inside a struct:
typedef struct { unsigned internal_state; } GRAPHEME_STATE;
the benefit of this is that the type GRAPHEME_STATE clearly states the
purpose, whereas a `uint_least16_t` doesn't.
Wrapping an enum into a struct is also a common trick to get stronger
type-checking from the compiler; I don't think it matters in this case
though, since the state is always passed via pointer.
> and I want all of the semantics to be crystal clear to the end-user.
Other way of looking at it is that the state is an internal thing so the
user shouldn't be concerned about what's going on behind the scene.
> +static inline void
> +state_serialize(const struct character_break_state *in, uint_least16_t *out)
> +{
> + *out = (uint_least16_t)(in->prop & UINT8_C(0xFF)) |
> /* first 8 bits */
> +(uint_least16_t)(((uint_least16_t)(in->prop_set)) << 8) |
> /* 9th bit */
> +(uint_least16_t)(((uint_least16_t)(in->gb11_flag))<< 9) |
> /* 10th bit */
> +(uint_least16_t)(((uint_least16_t)(in->gb12_13_flag)) << 10);
> /* 11th bit */
> +}
> +
> +static inline void
> +state_deserialize(uint_least16_t in, struct character_break_state *out)
> +{
> + out->prop = in & UINT8_C(0xFF);
> + out->prop_set = in & (((uint_least16_t)(1)) << 8);
> + out->gb11_flag= in & (((uint_least16_t)(1)) << 9);
> + out->gb12_13_flag = in & (((uint_least16_t)(1)) << 10);
> +}
The `(uint_least16_t)1` casts don't really do much since `int` is
guaranteed to be 16bits anyways. But if you want to be explicit, you can
still use `UINT16_C(1)`, which is shorter thus less noisy, instead of
casting:
- out->prop_set = in & (((uint_least16_t)(1)) << 8);
+ out->prop_set = in & (UINT16_C(1) << 8);
I'd also return by value in these 2 functions. Expressions like these
are more clear compared to out pointers:
*s = state_deserialize();
state = state_serialize(*s);
- NRK
[hackers] [libgrapheme][PATCH] fix manpage
- to_case: there's no `len` parameter. it should be `srclen` and `dstlen`.
- is_case: `caselen` should be a pointer.
---
P.S: one more thing that caught my eye; the "next" manpages for the
codepoint versions states:
If len is set to SIZE_MAX the string str is interpreted to be
NUL-terminated and processing stops when a NUL-byte is
encountered.
is this correct? what if the integer contains a nul-byte?
it seems to be that it should be an integer (uint_least32_t) with the
value 0, not a nul-byte, which are different things.
man/template/is_case.sh | 4 ++--
man/template/to_case.sh | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/man/template/is_case.sh b/man/template/is_case.sh
index 8ef2869..3cf00e6 100644
--- a/man/template/is_case.sh
+++ b/man/template/is_case.sh
@@ -22,7 +22,7 @@ cat << EOF
.Sh SYNOPSIS
.In grapheme.h
.Ft size_t
-.Fn grapheme_is_${CASE}${SUFFIX} "const ${DATATYPE} *str" "size_t len" "size_t
caselen"
+.Fn grapheme_is_${CASE}${SUFFIX} "const ${DATATYPE} *str" "size_t len" "size_t
*caselen"
.Sh DESCRIPTION
The
.Fn grapheme_is_${CASE}${SUFFIX}
@@ -54,7 +54,7 @@ function returns
.Dv true
if the ${ARRAYTYPE}
.Va str
-is ${CASE}, otherwise
+is ${CASE}, otherwise
.Dv false .
.Sh SEE ALSO
.Xr grapheme_is_${CASE}${ANTISUFFIX} 3 ,
diff --git a/man/template/to_case.sh b/man/template/to_case.sh
index 89b8996..8f17e53 100644
--- a/man/template/to_case.sh
+++ b/man/template/to_case.sh
@@ -38,7 +38,7 @@ is set to
.Dv NULL .
.Pp
If
-.Va len
+.Va srclen
is set to
.Dv SIZE_MAX
(stdint.h is already included by grapheme.h) the ${ARRAYTYPE}
@@ -56,7 +56,7 @@ function returns the number of ${UNIT}s in the array resulting
from converting
.Va src
to ${CASE}, even if
-.Va len
+.Va dstlen
is not large enough or
.Va dest
is
--
2.35.1
Re: [hackers] [PATCH] [st] FAQ: libXft now supports BGRA glyphs
On Thu, Sep 15, 2022 at 01:31:02PM +, Tom Schwindl wrote: > -## BadLength X error in Xft when trying to render emoji > - > -Xft makes st crash when rendering color emojis with the following error: > - > -"X Error of failed request: BadLength (poly request too large or internal > Xlib length error)" > - Major opcode of failed request: 139 (RENDER) > - Minor opcode of failed request: 20 (RenderAddGlyphs) > - Serial number of failed request: 1595 > - Current serial number in output stream: 1818" > - > -This is a known bug in Xft (not st) which happens on some platforms and > -combination of particular fonts and fontconfig settings. > - > -See also: > -https://gitlab.freedesktop.org/xorg/lib/libxft/issues/6 > -https://bugs.freedesktop.org/show_bug.cgi?id=107534 > -https://bugzilla.redhat.com/show_bug.cgi?id=1498269 I don't think removing the entire section is the right idea, since there are distros which are still on older versions. > -The solution is to remove color emoji fonts or disable this in the fontconfig > -XML configuration. As an ugly workaround (which may work only on newer > -fontconfig versions (FC_COLOR)), the following code can be used to mask color > -fonts: Simply changing this paragraph to say that the solution is to upgrade libXft should be the proper way to go IMO. - NRK
Re: [hackers] [dmenu] tab-complete: figure out the size before copying || NRK
Hi Santtu, On Fri, Sep 02, 2022 at 02:58:15PM +0300, Santtu Lakkala wrote: > This should now be > text[cursor] = '\0'; You're totally correct. I noticed this too some time later after I had already sent the patch. I was about to send an amended patch now but looks like it's already fixed thanks to you! - NRK
Re: [hackers][surf][quit hotkey] Suggestion to remove quit() function parameters
On Tue, Aug 30, 2022 at 08:29:40PM +0430, Lancia Greggori wrote: > Hi, I have recently noticed that the quit() function in the patch > https://surf.suckless.org/patches/quit-hotkey/ has parameters that are not > being used: Those "patches" are community maintained. Feel free to push directly to the repo if you think there's an improvemnt to be made. Read this page on how-to: https://suckless.org/wiki/ > To further test my theory, I changed the above quit function to quit(void) > and surf compiled and ran without a problem. > > Wouldn't it be better to remove these parameters that are not being used > and instead just put void in their place? No, because the function pointers in `Key` expects a certain signature. https://git.suckless.org/surf/file/surf.c.html#l122 - NRK
Re: [hackers] [dwm][PATCH] applyrules: read rules[] with the `r` pointer directly
On Mon, Aug 29, 2022 at 05:49:06PM -0500, explosion0men...@gmail.com wrote:
> + for (r = [0]; r <= [LENGTH(rules) - 1]; r++) {
The `[n]` construct always looks silly to me; it could've simply been
`a + n`:
for (r = rules + 0; r < rules + LENGTH(rules); ++r)
And `+ 0` is oviously retarded so after removing that you get this,
which is much cleaner and noise free:
for (r = rules; r < rules + LENGTH(rules); ++r)
- NRK
Re: [hackers] [dwm][PATCH RESEND 0/2] Const-correctness fixes
Hi Quentin, On Mon, Aug 22, 2022 at 11:29:04AM +0200, Quentin Rameau wrote: > strings pointed to by XClassHint members are not modified, they're > copied when necessary. I think you're correct on this. The only reason I sent the patch was since the thread/topic was about const-correctness. And _if_ const-correctness is to be a goal then I think it should extend to string-literals as well since they're effectively `const`. If that's not a goal/priority then the current code is fine. - NRK
Re: [hackers] [dwm][PATCH RESEND 1/2] drw: Maintain const-correctness for fontconfig/Xft types
On Sun, Aug 21, 2022 at 11:08:53PM +0100, Chris Down wrote: > We retrieve these from their respective functions arguments as const, > and can trivially respect that. Hi Chris, There's one more instance that's missed by this patch, it gets caught if you try to compile with `-Wcast-qual`: drw.c: In function 'drw_text': drw.c:336:75: warning: cast discards 'const' qualifier from pointer target type [-Wcast-qual] 336 |usedfont->xfont, x, ty, (XftChar8 *)utf8str, utf8strlen); | ^ - NRK
Re: [hackers] [dwm][PATCH RESEND 0/2] Const-correctness fixes
On Sun, Aug 21, 2022 at 11:08:43PM +0100, Chris Down wrote:
> I originally sent these a few years ago and the reply was that
> const-correctness wasn't something cared about, but seeing other const
> changes in dwm, and purely const-correctness improvements in
> quark/st/libgrapheme, I figured maybe it's worth reevaluating and seeing
> if they are of interest now.
I would add the attached patch to the list as well.
String literals in C are of the type `char []`, however modifying them
is actually UB [0] and in practice it can even result in a segfault.
The `-Wwrite-strings` flag treats string literals as `const char []` and
thus warns about cases where a string literal gets assigned to a `char *`.
P.S: I didn't add the flag to config.mk/CFLAGS in the patch since
changing the default warning flags can be seen as an opinionated change.
[0]: https://port70.net/~nsz/c/c99/n1256.html#6.4.5p6
- NRK
>From 49148ee9fb0bba0e1e503f70016381a779672b36 Mon Sep 17 00:00:00 2001
From: NRK
Date: Mon, 22 Aug 2022 13:56:09 +0600
Subject: [PATCH] fix const-correctness reguarding string literals
String literals in C are of the type `char []`, however modifying them
is actually UB [0] and in practice it can even result in a segfault.
The `-Wwrite-strings` flag treats string literals as `const char []` and
thus warns about cases where a string literal gets assigned to a `char *`.
[0]: https://port70.net/~nsz/c/c99/n1256.html#6.4.5p6
---
dwm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/dwm.c b/dwm.c
index 253aba7..55dd68c 100644
--- a/dwm.c
+++ b/dwm.c
@@ -1808,7 +1808,8 @@ updatebars(void)
.background_pixmap = ParentRelative,
.event_mask = ButtonPressMask|ExposureMask
};
- XClassHint ch = {"dwm", "dwm"};
+ char s[] = "dwm";
+ XClassHint ch = { s, s };
for (m = mons; m; m = m->next) {
if (m->barwin)
continue;
--
2.35.1
Re: [hackers] [dwm][PATCH] config.def.h: add comment about symbol size
On 2022-08-19 00:22, Hiltjo Posthuma wrote: I already said I disagree with the second part of the patch. Oh, sorry. That wasn't very clear, I thought you meant the stpncpy part.
Re: [hackers] [quark][PATCH] Fix buffer over-read in decode()
On Tue, Aug 16, 2022 at 08:58:37PM +, HushBugger wrote:
> Thanks, I don't have a lot of practical C experience.
The reason for the cast is because is a poorly designed
library where the caller needs to ensure that the arg is representable
as an `unsigned char` (i.e 0 .. UCHAR_MAX) or as `EOF` [0].
for (s = src, i = 0; *s; s++, i++) {
- if (*s == '%' && (sscanf(s + 1, "%2hhx", ) == 1)) {
+ if (*s == '%' && isxdigit((unsigned char)s[1]) &&
+ isxdigit((unsigned char)s[2])) {
+ sscanf(s + 1, "%2hhx", );
dest[i] = n;
s += 2;
At a glance, the `s += 2` looks incorrect to me because it's reading 3
bytes, not 2. But then I saw that the for loop increments `s` as well.
IMO messing around with the loop counter in two places is not a good
idea because it makes the code harder to reason about than it needs to
be. I think the `s++` should be removed from the for loop and `s` should
be incremented as needed inside the loop instead.
- s += 2;
+ s += 3;
} else {
- dest[i] = *s;
+ dest[i] = *s++;
But this is mostly style nit, so feel free to ignore.
[0]: https://port70.net/~nsz/c/c11/n1570.html#7.4
- NRK
Re: [hackers] [[dwm][PATCH]] drw / utf8decode : simpler is better ?
On Tue, Aug 16, 2022 at 12:00:52AM +0200, Hiltjo Posthuma wrote:
> I'd like to keep these functions. drw.{c,h} and some util functions are shared
> between some projects.
All the utf8 functions in drw.c are `static`. They can't be used from
another TU, by default at least.
If they *are* being used by some project (after modifying the source to
remove the `static` qualifier) then I think it'd make more sense to move
those functions to util.c and expose them in the header.
- NRK
Re: [hackers] [[dwm][PATCH]] drw / utf8decode : simpler is better ?
On Tue, Jul 19, 2022 at 04:38:34PM +0200, nenesses wrote:
> -static size_t
> -utf8validate(long *u, size_t i)
> -{
> - if (!BETWEEN(*u, utfmin[i], utfmax[i]) || BETWEEN(*u, 0xD800, 0xDFFF))
> - *u = UTF_INVALID;
> - for (i = 1; *u > utfmax[i]; ++i)
> - ;
> - return i;
> -}
I wonder if the return value was ever used or was it just premature
functionality. Either ways, the patch looks good to me, but I'm not the
maintainer here.
- NRK
Re: [hackers] [dwm][PATCH] ensure layout symbol is properly nul-terminated
On Sat, Aug 13, 2022 at 08:47:41AM +0200, Stein Gunnar Bakkeby wrote: > Would it be easier to make the layout > symbol size 15 (one less than the monitor symbol)? Not really, since the src might still end up not containing the nul-byte, the 16th byte will have to nul-terminated anyways. For example: char src[4] = "abcd"; /* no nul-byte in here */ char dst[5]; // strncpy strncpy(dst, src, sizeof dst - 1); /* only copied 4 bytes. the 5th byte in dst needs to nul-terminated */ dst[sizeof dst - 1] = 0; // or using stpncpy *stpncpy(dst, src, sizeof dst - 1) = '\0'; Or if you can ensure the last byte in dst is set to nul at the beginning, and is never written to again in the entire program then you can probably skip the manual nul-termination. But IMO hunting down all instances where `dst` is modified is more effort and error-prone than just nul-terminating right at the call site. > Doesn't this affect readability? I find it more readable to have the nul-termination and copy done in a single statement. But readability is subjective, so there's no definitive answer to that. - NRK
Re: [hackers] [dwm][PATCH] ensure layout symbol is properly nul-terminated
On Sat, Aug 13, 2022 at 09:55:00AM +0200, Hiltjo Posthuma wrote: > I think its simpler to just add the comment if needed. That's an option, yes. And I've considered it too. But it gets messy with Unicode/UTF-8 where one visible character != one byte. If you still think that's the way to go, then that's fine by me. Just wanted to make sure that you're aware of the unicode issue. - NRK
Re: [hackers] A better mailing list web archiver for suckless.org ... ?
On Thu, Aug 11, 2022 at 01:41:00PM +0200, Thomas Oltmann wrote: > This concern is probably completely esoteric, but I can see > how the pointer could theoretically overflow on some weird system > where the kernel doesn't sit in the higher half of the address space ... I'm personally not too worried about overflow. The bigger worry would be compiler's optimizer producing bad code. Gcc already does various "optimization" with the assumption that signed overflow cannot occur [0]. Don't think they *currently* do anything like that for pointer arithmetic (and probably unlikely they will, as it'd break too much code). But this situation is easily avoided by subtracting the "current" pointer from the "end" pointer and checking weather there's enough space available or not; which is guaranteed to behave properly by the standard. Since the goal here can be achieved by a standard method without any downside, makes sense to use that instead. [0] interesting thread on the matter: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=30475 - NRK
Re: [hackers] A better mailing list web archiver for suckless.org ... ?
On Wed, Aug 10, 2022 at 09:29:43PM +0200, Thomas Oltmann wrote:
> I think we can all agree that the current web archive over at
> lists.suckless.org isn't all that great;
> Author names get mangled, the navigation is terrible, some messages
> are duplicated, some missing.
I've noticed the missing mails too.
> Is there currently any interest in such a project here?
If it'd be an improvement over the current system then I don't see why
not.
> So far, I've gone ahead and implemented a sort of proof-of-concept (at
> https://www.github.com/tomolt/mailarchiver).
Hmm, interesting source code. A couple observations:
0. `.POSIX` needs to be first non-comment line in the Makefile
1. L277: pointer arithmetic is only valid as long as the result is
within the array or just 1 past it.
2. L36: `mail` should be declared `static` as it's not used outside of
the TU.
Usage of memcpy for string copying is good to see. I think more C
programmers should start thinking of strings as buffers and tracking
their length as necessary. Which can both improve efficiency and reduce
chances of buffer mishandling.
But in the case of `encode_html()`, stpcpy is probably the proper
function to use.
Anyways, I've attached patches for all the above. The stpcpy change is
opinionated, so feel free to reject that.
And one more thing:
/* TODO we should probably handle EINTR and partial reads */
Best thing to do here is not using `read()` to begin with. Instead use
`mmap()` to map the file into a private buffer. Otherwise I think using
`fread` is also an (inferior) option, don't think you need to worry
about EINTR with fread.
- NRK
>From 2e16b5e88e2a9394c1b331d81994186151474391 Mon Sep 17 00:00:00 2001
From: NRK
Date: Thu, 11 Aug 2022 14:13:55 +0600
Subject: [PATCH 1/4] .POSIX must be the first non-comment line
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
from `make 1p make`:
> The application shall ensure that this special target is
> specified without prerequisites or commands. If it appears
> as the first non-comment line in the makefile, make shall
> process the makefile as specified by this section; other‐
> wise, the behavior of make is unspecified.
---
Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index bd4c260..851d368 100644
--- a/Makefile
+++ b/Makefile
@@ -1,10 +1,10 @@
+.POSIX:
+
include config.mk
SRC = mailarchiver.c
OBJ = ${SRC:.c=.o}
-.POSIX:
-
.PHONY: all clean install uninstall
all: mailarchiver
--
2.35.1
>From 128a4d8ad26ffbd46f320747f408ad5a1edd0bbc Mon Sep 17 00:00:00 2001
From: NRK
Date: Thu, 11 Aug 2022 14:19:33 +0600
Subject: [PATCH 2/4] fix pointer arithmetic
pointer arithmetic are only allowed to go 1 past the end of the array.
going any further than that is UB.
> If both the pointer operand and the result point to elements of the
> same array object, or one past the last element of the array object,
> the evaluation shall not produce an overflow; otherwise, the behavior
> is undefined.
ref: https://port70.net/~nsz/c/c11/n1570.html#6.5.6p8
---
mailarchiver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mailarchiver.c b/mailarchiver.c
index e2a37f5..9f6fbad 100644
--- a/mailarchiver.c
+++ b/mailarchiver.c
@@ -274,7 +274,7 @@ encode_html(int fd, char *str)
case '\t': memcpy(w, "", 24); w += 24; break;
default: *w++ = *r;
}
- if (w + 24 > buf + sizeof buf) {
+ if (buf + sizeof buf - w <= 24) {
write(fd, buf, w - buf);
w = buf;
}
--
2.35.1
>From 4e0bebc289d053c3fad8ea0f0fc401c0b18d3aef Mon Sep 17 00:00:00 2001
From: NRK
Date: Thu, 11 Aug 2022 14:21:22 +0600
Subject: [PATCH 3/4] use stpcpy instead of memcpy
it's nice to see usage of memcpy but in this case stpcpy is the proper
function to use.
and optimizing compilers are able to optimize the function away [^0] due
to the src being available at compile time, so there's no performance
benefit towards using memcpy here either.
[^0]: https://godbolt.org/z/r89q6oGre
---
mailarchiver.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/mailarchiver.c b/mailarchiver.c
index 9f6fbad..b05c7df 100644
--- a/mailarchiver.c
+++ b/mailarchiver.c
@@ -266,12 +266,12 @@ encode_html(int fd, char *str)
for (r = str, w = buf; *r; r++) {
switch (*r) {
- case '<': memcpy(w, "", 4); w += 4; break;
- case '>': memcpy(w, "", 4); w += 4; break;
- case '&': memcpy(w, "", 5); w += 5; break;
- case '"': memcpy(w, "", 6); w += 6; break;
- case '\n': memcpy(w, "\n\n", 7); w += 7; break;
- case '\t': memcpy(w, "", 24); w += 24; break;
+ case '<': w = stpcpy(w, ""); break;
+ case '>': w = stpcpy(w, ""); break;
+ case '&': w = stpcpy(w, ""); break;
+ case '"':
Re: [hackers] [libs-style][PATCH] codestyle: die should be before ecalloc
On Mon, Aug 08, 2022 at 07:47:50PM +0600, NRK wrote: > > die should be before ecalloc > > Why should it be before `ecalloc` ? There's prototype in util.h already. > So the order of definition is irrelevant. Ah, I missed the commit `32fa3f5a code-style: sort function alphabetically`. Sorting alphabetically seems like a valid reason. Ignore my previous mail. - NRK
Re: [hackers] [libs-style][PATCH] codestyle: die should be before ecalloc
> die should be before ecalloc Why should it be before `ecalloc` ? There's prototype in util.h already. So the order of definition is irrelevant. - NRK
Re: [hackers] [libgrapheme] Rename reallocarray() to reallocate_array() to prevent mangling || Laslo Hunhold
On Sun, Jul 31, 2022 at 11:47:40AM +0200, g...@suckless.org wrote: > static void * > -reallocarray(void *p, size_t len, size_t size) > +reallocate_array(void *p, size_t len, size_t size) Given that this no longer shadows a libc/conventional function, I'd go one step further and move the `fprintf + exit` check inside reallocate_array() so the calling code doesn't need to worry about null returns. - NRK
Re: [hackers] [dwm][PATCH] spawn: reduce 2 lines, change fprintf() + perror() + exit() to die("... :")
On Sun, Jul 31, 2022 at 01:00:27AM +0200, Laslo Hunhold wrote: > as far as I can tell this is not correct, given the program exits with > EXIT_SUCCESS, not EXIT_FAILURE. Any specific reason why this matters? If exec(3) fails, I would assume a non-zero exit status would be more appropriate. Though I'm not even sure if this exit status ever even comes into play or not, since it's the exit status of the forked child and not dwm itself. - NRK
Re: [hackers] [dmenu][PATCH] don't mangle CPPFLAGS with CFLAGS
On Mon, Jul 04, 2022 at 03:01:35PM +0600, NRK wrote: > currently, doing something like the following would fail to build: > > $ make CFLAGS="-O3 -march=native" > > this is because CPPFLAGS get mangled with CFLAGS and so changing CFLAGS > kills the build. > > similarly it's conventional to have library flags in LDLIBS, so that > user can change LDFLAGS to add "-flto" or other flags they might want > without the build unnecessarily failing. > --- > > P.S: Could also drop `-std=c99` from CFLAGS and set CC to c99. But > didn't do that since it wasn't needed for my goal of being able to do > something like the following: > > $ make CFLAGS="-O3 -flto" LDFLAGS="-O3 -flto" > > Makefile | 8 +--- > config.mk | 6 +++--- > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/Makefile b/Makefile > index a03a95c..8d25ad0 100644 > --- a/Makefile > +++ b/Makefile > @@ -11,11 +11,13 @@ all: options dmenu stest > options: > @echo dmenu build options: > @echo "CFLAGS = $(CFLAGS)" > + @echo "CPPFLAGS = $(CPPFLAGS)" > @echo "LDFLAGS = $(LDFLAGS)" > + @echo "LDLIBS = $(LDLIBS)" > @echo "CC = $(CC)" > > .c.o: > - $(CC) -c $(CFLAGS) $< > + $(CC) -c $(CPPFLAGS) $(CFLAGS) $< > > config.h: > cp config.def.h $@ > @@ -23,10 +25,10 @@ config.h: > $(OBJ): arg.h config.h config.mk drw.h > > dmenu: dmenu.o drw.o util.o > - $(CC) -o $@ dmenu.o drw.o util.o $(LDFLAGS) > + $(CC) -o $@ dmenu.o drw.o util.o $(LDFLAGS) $(LDLIBS) > > stest: stest.o > - $(CC) -o $@ stest.o $(LDFLAGS) > + $(CC) -o $@ stest.o $(LDFLAGS) $(LDLIBS) > > clean: > rm -f dmenu stest $(OBJ) dmenu-$(VERSION).tar.gz > diff --git a/config.mk b/config.mk > index b0bd246..a513b74 100644 > --- a/config.mk > +++ b/config.mk > @@ -24,9 +24,9 @@ INCS = -I$(X11INC) -I$(FREETYPEINC) > LIBS = -L$(X11LIB) -lX11 $(XINERAMALIBS) $(FREETYPELIBS) > > # flags > -CPPFLAGS = -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE=700 > -D_POSIX_C_SOURCE=200809L -DVERSION=\"$(VERSION)\" $(XINERAMAFLAGS) > -CFLAGS = -std=c99 -pedantic -Wall -Os $(INCS) $(CPPFLAGS) > -LDFLAGS = $(LIBS) > +CPPFLAGS = $(INCS) -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE=700 > -D_POSIX_C_SOURCE=200809L -DVERSION=\"$(VERSION)\" $(XINERAMAFLAGS) > +CFLAGS = -std=c99 -pedantic -Wall -Os > +LDLIBS = $(LIBS) > > # compiler and linker > CC = cc > -- > 2.35.1 > > Any comment on this? Or any specific reason for deviating from the conventional approach and mangling `CFLAGS` and `CPPFLAGS`? - NRK
Re: [hackers] [dwm] Revert "do not call signal-unsafe function inside sighanlder" || Hiltjo Posthuma
> void
> sigchld(int unused)
> {
> + if (signal(SIGCHLD, sigchld) == SIG_ERR)
> + die("can't install SIGCHLD handler:");
> while (0 < waitpid(-1, NULL, WNOHANG));
> }
Calling `die` inside a signhandler is still an issue and can produce
bad behavior (I have seen a couple software which randomly freeze due to
calling signal unsafe functions inside a sighandler).
If `sigaction` can fix the issue with `signal` then it's probably best
to use that. Otherwise replacing `die` with `write` and `_exit` is also
a viable solution as shown in:
https://lists.suckless.org/hackers/2207/18405.html
- NRK
Re: [hackers] [dwm][PATCH] do not call signal-unsafe function inside sighanlder
On Tue, Jul 19, 2022 at 12:05:48AM +0200, François-Xavier Carton wrote:
> Technically, no standard guarantees that the handler stays after being
> called once. The implementation could reset it to the default action.
Hmm, didn't know about that.
> One may want to re-add a call to signal here, without error checking, to
> ensure the handler remains in case SIGCHLD is raised multiple times.
Alternatively we could revert this patch and then change the `die` call
to `write` and `_exit`; both of which are async-signal-safe.
void
sigchld(int unused)
{
if (signal(SIGCHLD, sigchld) == SIG_ERR) {
char errmsg[] = "Can't install SIGCHLD handler";
write(2, errmsg, sizeof errmsg);
_exit(1);
}
while (0 < waitpid(-1, NULL, WNOHANG));
}
This can't call `perror` similar to `die` as that's not signal safe.
- NRK
Re: [hackers] [dwm][patch] dwm-vertical-horizontal-i3like
On Mon, Jul 11, 2022 at 02:01:50PM +0700, Alif Radhitya wrote: > Assalamualaikum and good morning from Indonesia, Suckless Team. > I'm Alif Radhitya, another suckless user. > > I created a horizontal and vertical layout with one button like i3 for dwm. Think this should probably go to the wiki. You can push the patch there directly, see: https://suckless.org/wiki/ - NRK
Re: [hackers] [sent] [PATCH 1/3] sent.c: Drop unnecessary NULL checks
Hi Laslo, On Sun, Jun 26, 2022 at 11:16:32PM +0200, Laslo Hunhold wrote: > come on, it is general knowledge that free() accepts NULL arguments. I don't think that's a true. If it were general knowledge then we wouldn't see the pattern of `if (ptr) free(ptr)` in the code to begin with. And overall, I don't believe most "resource-release" functions have this behavior. For example fclose, closedir, munmap etc don't accept NULL arg. So `free` is more of an exception to the general knowledge. - NRK
Re: [hackers] [sent] [PATCH] treewide: Improve compliance with our coding style
On Sat, Jun 25, 2022 at 05:25:54PM +, Tom Schwindl wrote:
> -static void run();
> -static void usage();
> -static void xdraw();
> -static void xhints();
> -static void xinit();
> -static void xloadfonts();
> +static void run(void);
> +static void usage(void);
> +static void xdraw(void);
> +static void xhints(void);
> +static void xinit(void);
> +static void xloadfonts(void);
Functions with unspecified arguments is legacy cruft and obsolete since
C99. I'd perhaps go one step further and add `-Wstrict-prototypes` and
`-Wold-style-definition` to the list of default warnings in the
Makefile.
> - for (j = 0; j < LEN(fontfallbacks); j++)
> + for (j = 0; j < LEN(fontfallbacks); j++) {
> if (fstrs[j])
> free(fstrs[j]);
> + }
free on NULL is defined to be no-op. The check could be dropped.
- NRK
Re: [hackers] [libgrapheme] Add Word-data-files || Laslo Hunhold
On Fri, Jun 10, 2022 at 09:16:03PM +0200, g...@suckless.org wrote: > Even though the build works fine without them, the release tarball > should not depend on an internet connection and be self-contained. IMO they add unnecessary noise to the repo and commit diff. If this was the primary reason, then simply including them in the tarball would've sufficed. However since they already got committed, don't think it's worth reverting now. - NRK
Re: [hackers] [libgrapheme] Implement word-segmentation || Laslo Hunhold
On Mon, Jun 06, 2022 at 10:40:33PM +0200, g...@suckless.org wrote:
> + /* with no breaks we break at the end */
> + if (off == len) {
> + return len;
> + } else {
> + return off;
> + }
This is just the same as `return off;` , is it not?
- NRK
Re: [hackers] [dwm|dmenu|st][PATCH] strip the installed binary
On Mon, May 02, 2022 at 01:37:26PM +0200, Hiltjo Posthuma wrote:
> LDFLAGS="-s" is practically the same as calling strip and stripping it.
>
> It is up to the distro package/ports maintainer to strip symbols (or not).
> This can be an additional packaging step.
Since many of the Makefiles have "-Os" by default, I thought it would
make sense to go all the way and strip by default as.
> I'd rather have it so the Makefile respects the system or package system
> CFLAGS
> and LDFLAGS by default. Then someone can do: make CFLAGS="-Os" LDFLAGS="-s"
> etc.
Currently, the dwm and dmenu doesn't exactly "respect the system
{C,CPP,LD}FLAGS". ST however does:
# flags
STCPPFLAGS = -DVERSION=\"$(VERSION)\" -D_XOPEN_SOURCE=600
STCFLAGS = $(INCS) $(STCPPFLAGS) $(CPPFLAGS) $(CFLAGS)
STLDFLAGS = $(LIBS) $(LDFLAGS)
Changing dwm and dmenu to do something similar to ST seems fine to me.
> As a off-topic side-note I think we removing config.mk and just having the
> Makefile is simpler too.
I personally find the separation of config.mk and Makefile an annoyance.
But at the same time, I've seen many (novice) users who prefer having a
config.mk.
- NRK
Re: [hackers] [dmenu] fix incorrect comment, math is hard || Hiltjo Posthuma
Hi Quentin, On Sat, Apr 30, 2022 at 05:07:32PM +0200, Quentin Rameau wrote: > - inputw = mw / 3; /* input width: ~33% of monitor width */ > + inputw = mw / 3; /* input width: ~33.333% of monitor width */ You tried. diff --git a/dmenu.c b/dmenu.c index 571bc35..3595267 100644 --- a/dmenu.c +++ b/dmenu.c @@ -673,7 +673,7 @@ setup(void) mw = wa.width; } promptw = (prompt && *prompt) ? TEXTW(prompt) - lrpad / 4 : 0; - inputw = mw / 3; /* input width: ~33.333% of monitor width */ + inputw = mw / 3; /* input width: 1/3rd of monitor width */ match(); /* create menu window */ - NRK
Re: [hackers] [dmenu] inputw: improve correctness and startup performance, by NRK || Hiltjo Posthuma
On Sat, Apr 30, 2022 at 01:28:49PM +0200, Hiltjo Posthuma wrote: > Whats your monitor resolution width? I'm currently using 1366x768. Don't think it's worth optimizing for that resolution as most people use 1080p nowadays. > Whats your suggestion to improve it in a general sense? > > The trade-off is now that for small input strings in horizontal mode it might > wastes some visual space now, but like you said it is now consistently in the > same place and also simpler. There's no right or wrong answer here, since it'll basically come down to personal preference. I'd say making it a config.h option (and maybe adjustable via cli as well) would be a good course of action. It lets people easily change it if they don't like the default. - NRK
Re: [hackers] [dmenu] inputw: improve correctness and startup performance, by NRK || Hiltjo Posthuma
On Sat, Apr 30, 2022 at 11:15:38AM +0200, Hiltjo Posthuma wrote: > The previous maximum width also used about 30% of the monitor width. Should've clarified, I didn't quite like the previous max either. But that wouldn't get triggered before on small input strings. But since the width is static now, I didn't want it taking up 33% all the time. - NRK
Re: [hackers] [dmenu] inputw: improve correctness and startup performance, by NRK || Hiltjo Posthuma
On Fri, Apr 29, 2022 at 08:19:20PM +0200, g...@suckless.org wrote:
> commit e1e1de7b3b8399cba90ddca9613f837b2dbef7b9
> Author: Hiltjo Posthuma
> AuthorDate: Fri Apr 29 20:15:48 2022 +0200
> Commit: Hiltjo Posthuma
> CommitDate: Fri Apr 29 20:18:02 2022 +0200
>
> inputw: improve correctness and startup performance, by NRK
>
> Always use ~30% of the monitor width for the input in horizontal mode.
>
> Patch adapted from NRK patches.
> This also does not calculate inputw when using vertical mode anymore
> (because
> the code is removed).
>
> diff --git a/dmenu.c b/dmenu.c
> index 839f6cc..4e286cf 100644
> --- a/dmenu.c
> +++ b/dmenu.c
> @@ -610,13 +610,12 @@ static void
> setup(void)
> {
> int x, y, i, j;
> - unsigned int du, tmp;
> + unsigned int du;
> XSetWindowAttributes swa;
> XIM xim;
> Window w, dw, *dws;
> XWindowAttributes wa;
> XClassHint ch = {"dmenu", "dmenu"};
> - struct item *item;
> #ifdef XINERAMA
> XineramaScreenInfo *info;
> Window pw;
> @@ -674,12 +673,7 @@ setup(void)
> mw = wa.width;
> }
> promptw = (prompt && *prompt) ? TEXTW(prompt) - lrpad / 4 : 0;
> - for (item = items; item && item->text; ++item) {
> - if ((tmp = textw_clamp(item->text, mw/3)) > inputw) {
> - if ((inputw = tmp) == mw/3)
> - break;
> - }
> - }
> + inputw = mw / 3; /* input width: ~30% of monitor width */
> match();
>
> /* create menu window */
>
Hi Hiltjo,
Thanks for getting on this quickly.
I prefered the static bar width as it gives a predictable UI, the first
option will always appear in the same place rather than jumping around
depending on the input strings. And the fact that there was no way to
calculate the width correctly without taking unreasonable performance
hit anyways.
Though I must admit I'm having some second thoughts about it now. Given
that not taking fallback fonts "worked fine" (haven't seen anyone
complain) for about 6 years, maybe that wasn't too bad of an option.
But in any case, I'm fine with the current patch too, though I've
changed it to 1/4th of the monitor width rather than 1/3rd as I found
that too wide for my liking.
- NRK
Re: [hackers] [dmenu] inputw: improve correctness and startup performance || NRK
On Fri, Apr 29, 2022 at 04:57:20PM +0200, Laslo Hunhold wrote: > What's your stance on Wayland-support in dmenu? Would you accept a > patch? Hi Laslo, While you've asked this to Hiltjo, I figured I'd give my 2c on this since I've been trolling around the dmenu code base a bit recently. Most of the heavy-lifting is currently done via libsl, however libsl is a pretty thin abstraction over X and exposes a lot of the X (and Xft) specific details in the API. Just taking a look at `drw.h` should confirm this. So in order to support wayland, the entire API will need to reworked to hide away all low level details so that it can be used for both X and Wayland. But that's not all, dmenu itself makes a good amount of calls to Xlib functions. So all those will need to be abstracted away as well. At the end, I suspect it'll be much simpler to just have a separate branch or even just a rewrite from scratch rather than trying to cram support for both wayland and X in the existing codebase. - NRK
Re: [hackers] [dmenu] inputw: improve correctness and startup performance || NRK
On Fri, Apr 29, 2022 at 08:11:46AM +0200, Laslo Hunhold wrote:
> to keep a long story based on my experience with developing libgrapheme
> and intensely working with Unicode short: The char-width-data from the
> Unicode consortium cannot be relied on and said consortium has pretty
> much given up on this matter and delegated this to font-rendering and
> font-shaping implementations. While they still maintain the EAW-tables
> (among others), they are nothing more than heuristics that break
> horribly in many cases.
In that case, I think it's best to rule option 3 (codepoint width) and 2
(strlen) entirely.
I forgot to add the patch for option 1 (not taking fallback fonts into
account). Attached it here, it should apply cleanly on top of the patch
I sent earlier regarding not calculating `inputw` when using vertical
mode.
- NRK
>From 820a56238fdcc392442fc35026915abf7f8fad52 Mon Sep 17 00:00:00 2001
From: NRK
Date: Fri, 29 Apr 2022 09:03:48 +0600
Subject: [PATCH] inputw: stop taking fallback font into account
incorrect, but taking fallback fonts into account causes massive startup
slowdown when the input strings have many missing glyphs.
---
dmenu.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/dmenu.c b/dmenu.c
index 6b02b2b..75a81c2 100644
--- a/dmenu.c
+++ b/dmenu.c
@@ -675,7 +675,9 @@ setup(void)
}
promptw = (prompt && *prompt) ? TEXTW(prompt) - lrpad / 4 : 0;
for (item = items; !lines && item && item->text; ++item) {
- if ((tmp = textw_clamp(item->text, mw/3)) > inputw) {
+ drw_font_getexts(drw->fonts, item->text, strlen(item->text), , NULL);
+ tmp = MIN(mw/3, tmp + lrpad);
+ if (tmp > inputw) {
if ((inputw = tmp) == mw/3)
break;
}
--
2.35.1
Re: [hackers] [dmenu] inputw: improve correctness and startup performance || NRK
On Fri, Mar 25, 2022 at 10:57:42PM +0100, g...@suckless.org wrote:
> commit 77526f756e23e362081ac807521f901f2e5cd5e6
> Author: NRK
> AuthorDate: Thu Mar 24 00:37:55 2022 +0600
> Commit: Hiltjo Posthuma
> CommitDate: Fri Mar 25 22:49:07 2022 +0100
>
> inputw: improve correctness and startup performance
>
> a massive amount of time inside readstdin() is spent trying to get the
> max input width and then put it into inputw, only for it to get clamped
> down to mw/3 inside setup().
>
> it makes more sense to calculate inputw inside setup() once we have mw
> available. similar to the last patch, i see noticeable startup
> performance improvement:
>
> before -> after
> 160ms -> 60ms
>
> additionally this will take fallback fonts into account compared to the
> previous version, so it's not only more performant but also more correct.
>
> diff --git a/dmenu.c b/dmenu.c
> index cde394b..d989d39 100644
> --- a/dmenu.c
> +++ b/dmenu.c
> @@ -547,8 +547,7 @@ static void
> readstdin(void)
> {
> char buf[sizeof text], *p;
> - size_t i, imax = 0, size = 0;
> - unsigned int tmpmax = 0;
> + size_t i, size = 0;
>
> /* read each line from stdin and add it to the item list */
> for (i = 0; fgets(buf, sizeof buf, stdin); i++) {
> @@ -560,15 +559,9 @@ readstdin(void)
> if (!(items[i].text = strdup(buf)))
> die("cannot strdup %u bytes:", strlen(buf) + 1);
> items[i].out = 0;
> - drw_font_getexts(drw->fonts, buf, strlen(buf), , NULL);
> - if (tmpmax > inputw) {
> - inputw = tmpmax;
> - imax = i;
> - }
> }
> if (items)
> items[i].text = NULL;
> - inputw = items ? TEXTW(items[imax].text) : 0;
> lines = MIN(lines, i);
> }
>
> @@ -614,12 +607,13 @@ static void
> setup(void)
> {
> int x, y, i, j;
> - unsigned int du;
> + unsigned int du, tmp;
> XSetWindowAttributes swa;
> XIM xim;
> Window w, dw, *dws;
> XWindowAttributes wa;
> XClassHint ch = {"dmenu", "dmenu"};
> + struct item *item;
> #ifdef XINERAMA
> XineramaScreenInfo *info;
> Window pw;
> @@ -677,7 +671,12 @@ setup(void)
> mw = wa.width;
> }
> promptw = (prompt && *prompt) ? TEXTW(prompt) - lrpad / 4 : 0;
> - inputw = MIN(inputw, mw/3);
> + for (item = items; item && item->text; ++item) {
> + if ((tmp = textw_clamp(item->text, mw/3)) > inputw) {
> + if ((inputw = tmp) == mw/3)
> + break;
> + }
> + }
> match();
>
> /* create menu window */
>
Hi,
While this patch improves startup speed on larger strings, it caused a
startup performance regression when dealing with lots of small strings
*with* many missing glyphs[0].
Because this patch correctly takes fallback font into account, it'll
inevitably end up making lots of `XftFontMatch` calls, which are deadly
slow.
For example, trying to load this [1] list of emojis on my machine now
takes slightly more than half a second. Whereas before it took about
16ms.
The following are a couple different solutions I can think of:
1. (Incorrectly) stop taking fallback font into account.
2. (Incorrectly) assume `more bytes == wider string`, which is not
correct thanks to unicode.
3. Try to get the width of the unicode code-point. I've attached a quick
and dirty patch using `utf8proc_charwidth()` from libutf8-proc. The
patch was just to confirm my hypothesis and not to be taken seriously.
I'm not too well versed on unicode so I cannot tell how difficult
rolling such a function ourself would be. But some quick searches seems
to indicate it's not going to be trivial at all, specially if we take
"grapheme clusters" into account.
So this option is probably getting ruled out.
4. The final option; just remove the dynamic input bar width entirely.
If you're unsure what I'm talking about, try the following:
head -c 65536 < /dev/urandom | base64 | fold -w 1 | dmenu
You will notice that the width of the input bar is rather small, which
allows for more options to fit into the screen.
Now try replacing `-w 1` with `-w 30`, you will notice the width got
bigger. Now try `-w 200`, the width gets bigger, but will get clamped
down to 1/3rd of the monitor width.
My suggestion here is to just have a consistent input bar width which
can be configured via config.h and cli arg. So for example:
static float input_bar_percent =
Re: [hackers] [st][PATCH v2] code-golfing: cleanup osc color related code
Hi, Any remaining problem with the patch? Assuming it just fell off the radar :) - NRK
Re: [hackers] [libsl|dmenu][PATCH v2] Fix truncation issues and improve performance
On Tue, Mar 29, 2022 at 07:33:27PM +0200, Quentin Rameau wrote:
> > On Mon, Mar 28, 2022 at 05:57:48PM +0600, NRK wrote:
> > > And on the topic of ellipsis_width, we currently don't account for
> > > fallback font:
> > >
> > > usedfont = drw->fonts;
> > > drw_font_getexts(usedfont, "...", 3, _width, NULL);
> > >
> > > The assumption here was that every font should have '.' but if that
> > > turns out to be wrong, then the font width will be incorrect.
>
> I think that we can have as a requirement that the system provides a
> font with the glyph for dot.
The issue was less about what's available *on the system* and more about
what was in `fonts[0]`.
I've looked into this matter a bit and turns out there's many icon/emoji
fonts which *intentionally* don't provide any latin (or other language)
glyphs, for example "Font Awesome".
Currently, dwm/dmenu allows for doing things like this:
static const char *fonts[] = {
"Icon Font", /* good icons, but does not have latin glyphs */
"Mono Font", /* has icons, but they're crappy looking */
};
In this case, "Icon Font" would be used for icons, while "Mono Font"
would be used as fallback for any latin chars. Swapping "Mono Font" to
be `fonts[0]` won't quite work because then, "Mono Font" will end up
getting used for icons as well.
The existing code assumes `fonts[0]` has `.` glyph and doesn't check for
any of the fallback fonts. The earlier patch I sent solves this issue,
and will now correctly calculate the `ellipsis_width` taking fallback
fonts into account.
- NRK
Re: [hackers] [libsl|dmenu][PATCH v2] Fix truncation issues and improve performance
On Tue, Mar 29, 2022 at 05:18:12PM +0200, Stein Gunnar Bakkeby wrote:
> For 0002 how about leaving the ellipsis_width as 0 and do this?
>
> if (!ellipsis_width && render)
> ellipsis_width = drw_fontset_getwidth(drw, ellipsis);
When invoked from drw_fontset_getwidth(), `render` wont be active, so
yeah this will get rid of the infinite recursion as well. And
ellipsis_width is only relevant when rendering, so it makes sense to do
it this way.
In fact, I think just reducing the condition to `if (render)` and
reverting ellipsis_width to be automatic variable again would work as
well. But since ellipsis_width won't change, makes sense to make it
static and calculate it only once.
> Moving the "..." to a static(?) const variable named ellipsis would make it
> easier to replace this with something else (like the ellipsis character for
> example).
Currently it's used only in 2 locations, so don't think it should be
hard for someone to change it if they wish.
Attached the amended patches.
- NRK
>From cdf867f85a2aef1d43c0e1313d788646477884ab Mon Sep 17 00:00:00 2001
From: NRK
Date: Mon, 28 Mar 2022 01:02:52 +0600
Subject: [PATCH 1/2] drw_text: don't segfault when called with 0 width
this patch just rejects *any* 0 width draws, which is surely an error by
the caller.
this also guards against cases where the width is too small for the
ellipsis to fit, so ellipsis_w will remain 0.
reported by Bakkeby
---
drw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drw.c b/drw.c
index a50c9ee..2f3a5df 100644
--- a/drw.c
+++ b/drw.c
@@ -267,7 +267,7 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned int h, unsigned int lp
enum { nomatches_len = 64 };
static struct { long codepoint[nomatches_len]; unsigned int idx; } nomatches;
- if (!drw || (render && !drw->scheme) || !text || !drw->fonts)
+ if (!drw || (render && (!drw->scheme || !w)) || !text || !drw->fonts)
return 0;
if (!render) {
--
2.35.1
>From 5c4f072aef6c7e52f02dc421c164e5d3401174f2 Mon Sep 17 00:00:00 2001
From: NRK
Date: Mon, 28 Mar 2022 21:38:49 +0600
Subject: [PATCH 2/2] drw_text: account for fallback fonts in ellipsis_width
additionally, ellipsis_width (which shouldn't change) is made static to
avoid re-calculating it on each drw_text() call.
---
drw.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drw.c b/drw.c
index 2f3a5df..ced7d37 100644
--- a/drw.c
+++ b/drw.c
@@ -252,7 +252,7 @@ int
drw_text(Drw *drw, int x, int y, unsigned int w, unsigned int h, unsigned int lpad, const char *text, int invert)
{
int i, ty, ellipsis_x = 0;
- unsigned int tmpw, ew, ellipsis_w = 0, ellipsis_len, ellipsis_width;
+ unsigned int tmpw, ew, ellipsis_w = 0, ellipsis_len;
XftDraw *d = NULL;
Fnt *usedfont, *curfont, *nextfont;
int utf8strlen, utf8charlen, render = x || y || w || h;
@@ -266,6 +266,7 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned int h, unsigned int lp
/* keep track of a couple codepoints for which we have no match. */
enum { nomatches_len = 64 };
static struct { long codepoint[nomatches_len]; unsigned int idx; } nomatches;
+ static unsigned int ellipsis_width = 0;
if (!drw || (render && (!drw->scheme || !w)) || !text || !drw->fonts)
return 0;
@@ -283,7 +284,8 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned int h, unsigned int lp
}
usedfont = drw->fonts;
- drw_font_getexts(usedfont, "...", 3, _width, NULL);
+ if (!ellipsis_width && render)
+ ellipsis_width = drw_fontset_getwidth(drw, "...");
while (1) {
ew = ellipsis_len = utf8strlen = 0;
utf8str = text;
--
2.35.1
Re: [hackers] [libsl|dmenu][PATCH v2] Fix truncation issues and improve performance
On Tue, Mar 29, 2022 at 07:33:27PM +0200, Quentin Rameau wrote:
> > @@ -283,7 +284,10 @@ drw_text(Drw *drw, int x, int y, unsigned int w,
> > unsigned int h, unsigned int lp
> > }
> >
> > usedfont = drw->fonts;
> > - drw_font_getexts(usedfont, "...", 3, _width, NULL);
> > + if (ellipsis_width < 0) {
> > + ellipsis_width = 0; /* stop infinite recursion */
> > + ellipsis_width = drw_fontset_getwidth(drw, "...");
>
> I don't understand how setting it twice in a row stops a recursion.
> Will not the drw_fontset_getwidth() call *always* overwrite any value set
> before?
Hi Quentin,
drw_fontset_getwidth() is just a wrapper around drw_text()
unsigned int
drw_fontset_getwidth(Drw *drw, const char *text)
{
if (!drw || !drw->fonts || !text)
return 0;
return drw_text(drw, 0, 0, 0, 0, 0, text, 0);
}
so by setting ellipsis_width to 0, we make sure it skips this branch on
the recursive call due to the `if (ellipsis_width < 0)`.
Also, related to the 0 ellipsis_width patch, I'm thinking it might be
better to guard against any 0 width draws *in general* instead of just
for ellipsis_width. So something like this instead:
diff --git a/drw.c b/drw.c
index 96d868d..0cc5cbc 100644
--- a/drw.c
+++ b/drw.c
@@ -268,7 +268,7 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned
int h, unsigned int lp
static struct { long codepoint[nomatches_len]; unsigned int idx; }
nomatches;
static int ellipsis_width = -1;
- if (!drw || (render && !drw->scheme) || !text || !drw->fonts)
+ if (!drw || (render && (!drw->scheme || !w)) || !text || !drw->fonts)
return 0;
if (!render) {
- NRK
Re: [hackers] [libsl|dmenu][PATCH v2] Fix truncation issues and improve performance
On Mon, Mar 28, 2022 at 05:57:48PM +0600, NRK wrote:
> And on the topic of ellipsis_width, we currently don't account for
> fallback font:
>
> usedfont = drw->fonts;
> drw_font_getexts(usedfont, "...", 3, _width, NULL);
>
> The assumption here was that every font should have '.' but if that
> turns out to be wrong, then the font width will be incorrect.
Attached a patch fixing this issue, as well as a patch guarding against
calling drw_text() with 0 width.
Think both these issues should be extremely rare, but can't hurt dealing
with them anyways.
- NRK
>From 7c22fb59aedea1e7d65b1a8ef729dccd631a4364 Mon Sep 17 00:00:00 2001
From: NRK
Date: Mon, 28 Mar 2022 01:02:52 +0600
Subject: [PATCH 1/2] drw_text: guard against 0 ellipsis_w
if the width is too small for the ellipsis to fit, ellipsis_w will
remain 0.
reported by Bakkeby
---
drw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drw.c b/drw.c
index 4cccf47..411bcdd 100644
--- a/drw.c
+++ b/drw.c
@@ -336,7 +336,7 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned int h, unsigned int lp
x += ew;
w -= ew;
}
- if (render && overflow)
+ if (render && overflow && ellipsis_w)
drw_text(drw, ellipsis_x, y, ellipsis_w, h, 0, "...", invert);
if (!*text || overflow) {
--
2.35.1
>From 20a9736ea91a9801a6da3a6f829976e7d59a3a32 Mon Sep 17 00:00:00 2001
From: NRK
Date: Mon, 28 Mar 2022 21:38:49 +0600
Subject: [PATCH 2/2] drw_text: account for fallback fonts in ellipsis_width
this required using a static variable to stop infinite recursion.
as a side-effect, we now avoid doing some unneeded work by calculating
ellipsis_width (which shouldn't change) only once instead of calculating
it on each drw_text() call.
---
drw.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drw.c b/drw.c
index 411bcdd..96d868d 100644
--- a/drw.c
+++ b/drw.c
@@ -252,7 +252,7 @@ int
drw_text(Drw *drw, int x, int y, unsigned int w, unsigned int h, unsigned int lpad, const char *text, int invert)
{
int i, ty, ellipsis_x = 0;
- unsigned int tmpw, ew, ellipsis_w = 0, ellipsis_len, ellipsis_width;
+ unsigned int tmpw, ew, ellipsis_w = 0, ellipsis_len;
XftDraw *d = NULL;
Fnt *usedfont, *curfont, *nextfont;
int utf8strlen, utf8charlen, render = x || y || w || h;
@@ -266,6 +266,7 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned int h, unsigned int lp
/* keep track of a couple codepoints for which we have no match. */
enum { nomatches_len = 64 };
static struct { long codepoint[nomatches_len]; unsigned int idx; } nomatches;
+ static int ellipsis_width = -1;
if (!drw || (render && !drw->scheme) || !text || !drw->fonts)
return 0;
@@ -283,7 +284,10 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned int h, unsigned int lp
}
usedfont = drw->fonts;
- drw_font_getexts(usedfont, "...", 3, _width, NULL);
+ if (ellipsis_width < 0) {
+ ellipsis_width = 0; /* stop infinite recursion */
+ ellipsis_width = drw_fontset_getwidth(drw, "...");
+ }
while (1) {
ew = ellipsis_len = utf8strlen = 0;
utf8str = text;
--
2.35.1
Re: [hackers] [libsl|dmenu][PATCH v2] Fix truncation issues and improve performance
On Mon, Mar 28, 2022 at 10:01:10AM +0200, Stein Gunnar Bakkeby wrote: > That ellipsis_w guard makes sense to add. > > You are right in that it is simpler to call drw_text. If we make another > call to XftDrawStringUtf8 then we also need to recalculate ty. You'd also need to call XFillRectangle(...), otherwise the ellipsis will *overlap* what was previously there instead of *overwriting* it. And on the topic of ellipsis_width, we currently don't account for fallback font: usedfont = drw->fonts; drw_font_getexts(usedfont, "...", 3, _width, NULL); The assumption here was that every font should have '.' but if that turns out to be wrong, then the font width will be incorrect. I don't think we can call drw_fontset_getwidth() though, since that'll recurse back into drw_text(). - NRK
Re: [hackers] [libsl|dmenu][PATCH v2] Fix truncation issues and improve performance
On Sat, Mar 26, 2022 at 12:02:12AM +0100, Stein Gunnar Bakkeby wrote:
> for the first patch, what was the incentive for making a call to drw_text
> to draw the ellipsis rather than making another call to XftDrawStringUtf8?
Was simpler to just call drw_text.
> As-is, and I am primarily thinking of the bar in dwm in this case, if you
> were to have limited space and there is only enough space to write one or
> two characters then you have a situation where the ellipsis would never
> fit, but the text will overflow.
>
> In this situation the ellipsis_w variable will remain 0 and drw_text is
> called again with a width of 0 to draw the ellipsis. I believe it might
> crash with that.
Hmm, how about this:
diff --git a/drw.c b/drw.c
index 4cccf47..411bcdd 100644
--- a/drw.c
+++ b/drw.c
@@ -336,7 +336,7 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned
int h, unsigned int lp
x += ew;
w -= ew;
}
- if (render && overflow)
+ if (render && overflow && ellipsis_w)
drw_text(drw, ellipsis_x, y, ellipsis_w, h, 0, "...",
invert);
if (!*text || overflow) {
- NRK
Re: [hackers] [dmenu] Revert "avoid redraw when there's no change" || Hiltjo Posthuma
On Sun, Mar 27, 2022 at 12:11:01AM +0600, NRK wrote: > One thing I can think of, is just to keep track of the previous cursor > position within drawmenu() and return early if there's no change. > > I'll play around with this idea more thoroughly and see if there's any > edge cases where this doesn't work before sending the patch. Yeah.. this won't work. There's other things we'll need to keep track of as well such as current selction etc. Probably not worth it that that point; unless there's a simpler and cleaner solution. - NRK
Re: [hackers] [dmenu] Revert "avoid redraw when there's no change" || Hiltjo Posthuma
On Sat, Mar 26, 2022 at 06:00:03PM +0100, g...@suckless.org wrote:
> commit 31fa07b9849b0ffbf4b7efb55943f466b3ff160f
> Author: Hiltjo Posthuma
> AuthorDate: Sat Mar 26 17:57:50 2022 +0100
> Commit: Hiltjo Posthuma
> CommitDate: Sat Mar 26 17:57:50 2022 +0100
>
> Revert "avoid redraw when there's no change"
>
> This reverts commit 6818e07291f3b2913e687c8ec3d3fe4711724050.
>
> This broke keys such as ^W to delete-backward-word
>
> diff --git a/dmenu.c b/dmenu.c
> index 19f6385..085dc29 100644
> --- a/dmenu.c
> +++ b/dmenu.c
> @@ -415,9 +415,8 @@ keypress(XKeyEvent *ev)
> switch(ksym) {
> default:
> insert:
> - if (iscntrl((unsigned char)*buf))
> - return;
> - insert(buf, len);
> + if (!iscntrl(*buf))
> + insert(buf, len);
> break;
> case XK_Delete:
> case XK_KP_Delete:
>
The patch was definitely short-sighted and I should've tested it more.
I saw that drawing had it's own goto label and some of the above keys
were doing `goto draw` instead of relying on the code going down.
But even if we change, for example the ctrl+w, from `break` to `goto draw`:
case XK_w: /* delete word */
/* ... */
- break;
+ goto draw;
this still wouldn't have solved the problem because someone can press
ctrl+w on empty prompt, which effectively changes nothing, but still
would do a redraw.
Doing a redraw isn't THAT bad anymore with the recent patches, so maybe
this is a problem not worth solving. But even now, redrawing isn't
exactly "free".
One thing I can think of, is just to keep track of the previous cursor
position within drawmenu() and return early if there's no change.
I'll play around with this idea more thoroughly and see if there's any
edge cases where this doesn't work before sending the patch.
- NRK
Re: [hackers] [dmenu][PATCHes] Fix truncation issues and improve performance
On Wed, Mar 23, 2022 at 11:13:00AM +0100, Stein Gunnar Bakkeby wrote:
> Regarding the first patch I got the impression that the overflow should be
> triggered if (ew + tmpw > w) {
You're correct. I forgot to mention, but that was another bug I have
fixed locally.
> For the second patch I am not sure if this would be bad form or not, but
> the invert variable is only ever used when text is rendered so I was
> wondering if this might be re-used for the purposes of containing the
> minw value when it is calculating text width.
>
> w = invert ? invert : ~invert;
>
> This would avoid the wrapper function and change of signature and simplify
> the patch quite a bit. It would probably be best to rename the parameter in
> that case.
I've thought about that as well, but I don't think it's a good idea to
overload the variable with multiple meaning.
But then again library users should be calling drw_fontset_getwidth(),
so maybe it's fine to overload the variable for internal usage. If
that's deemed acceptable then I can change it.
Anyhow, I've been using the patches on both dwm and dmenu, and so far
everything seems to be working as expected :)
I'll wait until tomorrow and give the patches another look with fresh
eyes to see if there's some edge case unhandled before sending them.
- NRK
Re: [hackers] [dmenu][PATCHes] Fix truncation issues and improve performance
On Wed, Mar 23, 2022 at 09:13:52AM +0100, Hiltjo Posthuma wrote: > Please continue working on this patch, it is appreciated. > When it mostly works it can be put into libsl and dmenu and dwm. > > If possible please make the first iteration compatible with the current API. > This would make reviewing easier. > > Afterwards it's no issue (although preferably not) if some API changes are > needed. > > Reducing the changes would make this easier to review, as this code and also > Xft etc are finicky (as you probably can already tell :)). Thanks, will keep that in mind. There was a small rendering bug in the 1st patch, instead overwriting the rest of the area with the ellipsis, we were only overwriting ellipsis's width. I've fixed it locally to correctly keeps track of the width we need to overwrite as well. In the 2nd patch it's calling getwidth_clamp() inside MIN(..) macro, which meant it was probably needlessly calculating the width twice. I didn't even notice it because despite calculating width twice, it was still faster than the vanilla version. So far haven't found any issues with the 3rd patch, but in the commit msg I only mentioned performance improvement, however I unknowingly improved the correctness as well. drw_font_getexts() was introduced inside readstdin() in commit 44c7de3, which states: - fix bug where inputw was not correctly calculated from the widest item, but just from the one with the longest strlen() which is not the same. It's better now, but does not account for fallback fonts, since it would be too slow to calculate all the correct item widths on startup. In the 3rd patch, we take into account the fallback fonts as well. So not only was it faster, but it was also more correct. Finally, with these 3 patches applied, I can dump some decently sized (1500 bytes) emoji file into dmenu and not have it *FREEZE* when trying to scroll up and down. However it was still noticeably lagging. The offender? Once again drw_text(). This time the problem case was when we don't have any font in our font list which could render the char. In which case we call XftFontMatch() which is INSANELY slow. The main problem is that we're calling it *even* on codepoints where we *know* there is no match. The solution to that was just to keep a small cache which keeps track of codepoints where there's no match and avoid calling XftFontMatch in those cases. With the final piece of the puzzle solved, I can now deal with unicode files (with missing fonts) without any lag whatsoever. It feels just as fast as dealing with ascii text. There was a couple other _unrelated_ issues I noticed as well, will send patches for them later on. As for the current patches, I'll play around a bit more with them, look out for any lurking bugs and clean them up before sending again. - NRK
Re: [hackers] [dmenu][PATCHes] Fix truncation issues and improve performance
On Tue, Mar 22, 2022 at 06:06:30PM +0100, Stein Gunnar Bakkeby wrote: > With the first patch the text is still allowed to bleed into the right hand > side padding as long as it fits. > > I worked around that by reducing w with 2 * lpad and adding lpad to x > before returning. Ahh, sorry my bad. I missed the "adding lpad to return" part. Yeah, that seems to stop the prompt from getting cut off, but I as I said given the variable name `lpad`, I guess that's probably intentional. Let's wait for someone to clarify what the intentional behavior is. > I was thinking that the function drw_font_getexts might get removed, it was > probably used for more in previous iterations. I made the patches with the assumption that they could/would also be integrated into libsl[0]. So I didn't remove anything or break the API. [0]: https://git.suckless.org/libsl - NRK
Re: [hackers] [dmenu][PATCHes] Fix truncation issues and improve performance
On Tue, Mar 22, 2022 at 06:06:30PM +0100, Stein Gunnar Bakkeby wrote: > With the first patch the text is still allowed to bleed into the right hand > side padding as long as it fits. > > I worked around that by reducing w with 2 * lpad and adding lpad to x > before returning. Yeah, that's what was causing the prompt to get cut off. I assume you overlooked it last time I sent, but here's the reproduction step: ./dmenu < /dev/null -p text With the `w -= lpad * 2`, the prompt will get cut off. Also given the variable name is _l_pad, I think it's probably intentional for the text to bleed in to the right side. - NRK
Re: [hackers] [dmenu|libsl][PATCH] optimize drw_text() for large stringsa
On Mon, Mar 21, 2022 at 10:35:21PM +0100, Stein Gunnar Bakkeby wrote:
> you make some interesting points. I am curious as to what your queueing
> approach would look like.
>
> I played around some more simplifying the ellipsis drawing and removing buf
> as you suggested.
> This would solve all of the aforementioned problems as far as I can tell,
> but it can result in a partly drawn
> emoji for example when the ellipsis cuts it off (which I think is a fair
> tradeoff).
Hi,
Tried out your last patch, it's simpler indeed. But I still see the
prompt getting incorrectly cut off and the ellipsis still don't get
rendered in case of font change.
The queue patch solves the above problems, but as I've said it's quite
messy. I was planning to move everything over to the queue so that we
won't need to special case anything during string drawing.
But after looking at your current patch, I've scraped that idea. It's
simpler to just draw on top of the problem cases, overwriting them
instead.
The following diff fixes pretty much every problem for me. The
performance is better and I don't notice any problems with truncating.
Let me know if there's still some edge case unhandled.
- NRK
diff --git a/drw.c b/drw.c
index 4cdbcbe..8595a69 100644
--- a/drw.c
+++ b/drw.c
@@ -251,12 +251,10 @@ drw_rect(Drw *drw, int x, int y, unsigned int w, unsigned
int h, int filled, int
int
drw_text(Drw *drw, int x, int y, unsigned int w, unsigned int h, unsigned int
lpad, const char *text, int invert)
{
- char buf[1024];
- int ty;
- unsigned int ew;
+ int ty, ellipsis_x = 0;
+ unsigned int tmpw, ew, ellipsis_w, ellipsis_len;
XftDraw *d = NULL;
Fnt *usedfont, *curfont, *nextfont;
- size_t i, len;
int utf8strlen, utf8charlen, render = x || y || w || h;
long utf8codepoint = 0;
const char *utf8str;
@@ -264,7 +262,7 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned
int h, unsigned int lp
FcPattern *fcpattern;
FcPattern *match;
XftResult result;
- int charexists = 0;
+ int charexists = 0, overflow = 0;
if (!drw || (render && !drw->scheme) || !text || !drw->fonts)
return 0;
@@ -282,8 +280,9 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned
int h, unsigned int lp
}
usedfont = drw->fonts;
+ drw_font_getexts(usedfont, "...", 3, _w, NULL);
while (1) {
- utf8strlen = 0;
+ ew = ellipsis_len = utf8strlen = 0;
utf8str = text;
nextfont = NULL;
while (*text) {
@@ -291,9 +290,19 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned
int h, unsigned int lp
for (curfont = drw->fonts; curfont; curfont =
curfont->next) {
charexists = charexists ||
XftCharExists(drw->dpy, curfont->xfont, utf8codepoint);
if (charexists) {
- if (curfont == usedfont) {
+ drw_font_getexts(curfont, text,
utf8charlen, , NULL);
+ if (ew + ellipsis_w <= w) {
+ ellipsis_x = x + ew;
+ ellipsis_len = utf8strlen;
+ }
+
+ if (ew > w) {
+ overflow = 1;
+ utf8strlen = ellipsis_len;
+ } else if (curfont == usedfont) {
utf8strlen += utf8charlen;
text += utf8charlen;
+ ew += tmpw;
} else {
nextfont = curfont;
}
@@ -301,36 +310,25 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned
int h, unsigned int lp
}
}
- if (!charexists || nextfont)
+ if (overflow || !charexists || nextfont)
break;
else
charexists = 0;
}
if (utf8strlen) {
- drw_font_getexts(usedfont, utf8str, utf8strlen, ,
NULL);
- /* shorten text if necessary */
- for (len = MIN(utf8strlen, sizeof(buf) - 1); len && ew
> w; len--)
- drw_font_getexts(usedfont, utf8str, len, ,
NULL);
-
- if (len) {
- memcpy(buf, utf8str, len);
-
Re: [hackers] [dmenu|libsl][PATCH] optimize drw_text() for large strings
On Mon, Mar 21, 2022 at 07:00:32PM +0600, NRK wrote:
> The only "issue" is that it doesn't render the ellipsis in case font
> changes, but current upstream dmenu doesn't seem to do it either.
OK, I think I have a solution to this. The problem here, as far as I
understand, is this:
Let's say we have a maxwidth of 100, and printing the ellipsis takes 20.
In this case as long as our string is below 80, we're fine; and if it's
above 100 then we should print up to 80 and then print the ellipsis.
The problem case is when we're between 80-100, and we need to change
font. The current code always renders whatever is available when we
change font, so if the text turns out to be bigger than 100 we've
already rendered some of the text into the problem area where the
ellipsis should've been.
The solution I came up with is to simply not render anything if we're at
the problem case, and instead put the problem cases into a queue and
keep going forwards until either:
1) The text overflows, in which case we discard the queue and just print
the ellipsis instead.
2) Encounter the end of text, which means the text barely fit into our
maxwidth (this would happen with the prompt); in which case we just
render the queue and don't print any ellipsis.
I already have a patch that *roughly* does what I described above and
with it applied I don't see any truncation problems anymore.
However the patch is quite messy atm, and in order to do things cleanly
and avoid special casing, I think a good portion of drw_text() will need
to be overhauled to use a queue like this:
struct { const char *str; int len; Fnt *fnt; } q[32] = {0};
I'm not sure if such overhaul is going to be welcome or not, but if all
of this sounds acceptable then I can proceed with cleaning things up and
supplying the patch.
- NRK
Re: [hackers] [dmenu|libsl][PATCH] optimize drw_text() for large strings
+ if (ew + ext.xOff + lpad > w ||
b + utf8charlen > sizeof(buf) - 1) {
+ /* Only draw ellipsis
if we have not recently started another font */
+ if (render &&
ellipsis_b > 3) {
+ ew =
ellipsis_ew;
[...]
+ /* Record the last buffer index
where the ellipsis would still fit */
+ if (ew + ellipsis_width + lpad
<= w) {
+ ellipsis_ew = ew;
+ ellipsis_b = b;
+ }
I think both of the `+ lpad` needs be removed. Otherwise it incorrectly
truncates the prompt as well.
./dmenu < /dev/null -p "p" # empty prompt
./dmenu < /dev/null -p "prompt" # truncated prompt
Also, I didn't quite get why there's a `ellipsis_b > 3` in there.
+ for (i = 0; i < utf8charlen;
i++)
+ buf[b++] = *text++;
I'm kinda wondering if `buf` is even worth it or not. We could just render the
"..." separately. On my system atleast, there is no noticeable performance
difference, but removing `buf` from the equation (IMO) makes things more
simpler and easier to follow.
The following is what I've gotten so far, it's working fine and I haven't
noticed any regressions. The only "issue" is that it doesn't render the
ellipsis in case font changes, but current upstream dmenu doesn't seem to do it
either.
- NRK
diff --git a/drw.c b/drw.c
index 4cdbcbe..80dcad2 100644
--- a/drw.c
+++ b/drw.c
@@ -251,20 +251,17 @@ drw_rect(Drw *drw, int x, int y, unsigned int w, unsigned
int h, int filled, int
int
drw_text(Drw *drw, int x, int y, unsigned int w, unsigned int h, unsigned int
lpad, const char *text, int invert)
{
- char buf[1024];
- int ty;
- unsigned int ew;
+ unsigned int ew = 0, ellipsis_ew = 0, ellipsis_width = 0, tmpw;
XftDraw *d = NULL;
Fnt *usedfont, *curfont, *nextfont;
- size_t i, len;
- int utf8strlen, utf8charlen, render = x || y || w || h;
+ int utf8strlen, utf8charlen, ellipsis_len, render = x || y || w || h;
long utf8codepoint = 0;
const char *utf8str;
FcCharSet *fccharset;
FcPattern *fcpattern;
FcPattern *match;
XftResult result;
- int charexists = 0;
+ int charexists = 0, truncate = 0;
if (!drw || (render && !drw->scheme) || !text || !drw->fonts)
return 0;
@@ -283,7 +280,7 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned
int h, unsigned int lp
usedfont = drw->fonts;
while (1) {
- utf8strlen = 0;
+ utf8strlen = ellipsis_len = ew = ellipsis_ew = 0;
utf8str = text;
nextfont = NULL;
while (*text) {
@@ -292,8 +289,27 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned
int h, unsigned int lp
charexists = charexists ||
XftCharExists(drw->dpy, curfont->xfont, utf8codepoint);
if (charexists) {
if (curfont == usedfont) {
+ if (!ellipsis_width)
+
drw_font_getexts(curfont, "...", 3, _width, NULL);
+ drw_font_getexts(curfont, text,
utf8charlen, , NULL);
+ if (ew + tmpw > w) {
+ /* Only draw ellipsis
if we have not recently started another font */
+ if (render &&
ellipsis_len > 0) {
+ ew =
ellipsis_ew;
+ utf8strlen =
ellipsis_len;
+ }
+ truncate = 1;
+ break;
+ }
+
+ /* Record the last text index
where the ellipsis would still fit */
+ if (ew + ellipsis_width <= w) {
+ ellipsis_ew = ew;
+ ellipsis_len =
utf8strlen;
+
Re: [hackers] [st][PATCH v2] code-golfing: cleanup osc color related code
On Mon, Mar 21, 2022 at 09:24:50AM +0100, Roberto E. Vargas Caballero wrote:
> I think is better to keep every ternary in a line by itself, because it makes
> easier to read them:
>
> fprintf(stderr, "erresc: failed to fetch %s color %d\n",
> is_osc4 ? "osc4" : "osc",
> is_osc4 ? num : index);
Done.
> I think we force here to put braces around if and else because the body of the
> if part is more of one line. Again, I think is better to use a line for every
> ternary and have something like:
>
>
> if (n < 0 || n >= sizeof(buf)) {
> fprintf(stderr, "error: %s while printing %s response\n",
> n < 0 ? "snprintf failed" : "truncation occurred",
> is_osc4 ? "osc4" : "osc");
> } else {
> ttywrite(buf, n, 1);
> }
>
> > + if ((j = par - 10) < 0 || j >= LEN(osc_table))
> > + break; /* shouldn't be possible */
> >
> > if (!strcmp(p, "?"))
> > - osc_color_response(defaultcs, 12);
> > - else if (xsetcolorname(defaultcs, p))
> > - fprintf(stderr, "erresc: invalid cursor color:
> > %s\n", p);
> > + osc_color_response(par, osc_table[j].idx, 0);
> > + else if (xsetcolorname(osc_table[j].idx, p))
> > + fprintf(stderr, "erresc: invalid %s color:
> > %s\n",
> > + osc_table[j].str, p);
> > else
> > tfulldirt();
> > return;
>
> Same apply here, I think our style forces to have braces in every if and else
> if
> because there is a body with more of one lines.
I agree with that, but I wasn't sure what the preferred style here is
since I've seen a lot of multi-line bodies have no braces, most notably
on the DWM source code. Though, looking around a bit, the ST source
seems to be consistent with it's braces.
Attached the amended patch.
- NRK
>From cab20470fd6c6f3832f10427b1970df5acf78245 Mon Sep 17 00:00:00 2001
From: NRK
Date: Fri, 7 Jan 2022 23:21:04 +0600
Subject: [PATCH] code-golfing: cleanup osc color related code
* adds missing function prototype
* move xgetcolor() prototype to win.h (that's where all the other x.c
func prototype seems to be declared at)
* check for snprintf error/truncation
* reduces code duplication for osc 10/11/12
* unify osc_color_response() and osc4_color_response() into a single function
the latter two was suggested by Quentin Rameau in his patch review on
the hackers list.
---
st.c | 90 +--
st.h | 2 --
win.h | 1 +
3 files changed, 33 insertions(+), 60 deletions(-)
diff --git a/st.c b/st.c
index c71fa06..7275da5 100644
--- a/st.c
+++ b/st.c
@@ -161,6 +161,7 @@ static void csidump(void);
static void csihandle(void);
static void csiparse(void);
static void csireset(void);
+static void osc_color_response(int, int, int);
static int eschandle(uchar);
static void strdump(void);
static void strhandle(void);
@@ -1843,39 +1844,28 @@ csireset(void)
}
void
-osc4_color_response(int num)
+osc_color_response(int num, int index, int is_osc4)
{
int n;
char buf[32];
unsigned char r, g, b;
- if (xgetcolor(num, , , )) {
- fprintf(stderr, "erresc: failed to fetch osc4 color %d\n", num);
+ if (xgetcolor(is_osc4 ? num : index, , , )) {
+ fprintf(stderr, "erresc: failed to fetch %s color %d\n",
+ is_osc4 ? "osc4" : "osc",
+ is_osc4 ? num : index);
return;
}
- n = snprintf(buf, sizeof buf, "\033]4;%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
- num, r, r, g, g, b, b);
-
- ttywrite(buf, n, 1);
-}
-
-void
-osc_color_response(int index, int num)
-{
- int n;
- char buf[32];
- unsigned char r, g, b;
-
- if (xgetcolor(index, , , )) {
- fprintf(stderr, "erresc: failed to fetch osc color %d\n", index);
- return;
+ n = snprintf(buf, sizeof buf, "\033]%s%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
+ is_osc4 ? "4;" : "", num, r, r, g, g, b, b);
+ if (n < 0 || n >= sizeof(buf)) {
+ fprintf(stderr, "error: %s while printing %s response\n",
+ n < 0 ? "snprintf failed" : "truncation occurred",
+ is_osc4 ? "osc4" : "osc");
+ } else {
+ ttywrite(buf, n, 1);
}
-
- n = snprintf(buf, sizeof buf, "\033]%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
- num, r, r, g, g, b, b);
-
- ttywrite(buf, n, 1)
Re: [hackers] [dmenu|libsl][PATCH] optimize drw_text() for large strings
On Sun, Mar 20, 2022 at 02:56:33PM +0100, Stein Gunnar Bakkeby wrote: > I have attached the changes I was experimenting with. > I still don't like the amount of effort required for drawing an ellipsis > though so I attached another patch file that doesn't try to draw the > ellipsis for comparison. > > -Stein Hi Bakkeby, I haven't looked too deeply into the patch, but I see that you've moved the ew calculation inside the first loop rather than looping through again later down. This was more or less what I was planning to do as well, although I was motivated by performance reasons, while you seemed to be motivated by correctness reasons. Anyhow, it's a good thing that you've sent this mail as it would prevent some duplicated work on my end. As for your patch, I think it (just using a single loop instead of two) is a better approach than the one I sent, although the `ellipsis_width` shenanigan a bit questionable; I wonder if there's a cleaner way to do it. Anyhow, your patch also solves the performance problem with large strings on my end, and additionally I'm also seeing some startup time improvement when dumping large non-ascii strings into dmenu. - NRK
Re: [hackers] [st][PATCH v2] code-golfing: cleanup osc color related code
On Sun, Mar 20, 2022 at 12:49:22PM +0100, Hiltjo Posthuma wrote:
> > - n = snprintf(buf, sizeof buf,
> > "\033]4;%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
> > -num, r, r, g, g, b, b);
> > -
> > - ttywrite(buf, n, 1);
> > -}
>
> A nitpick, because I think the string always fits in the buffer `buf`, but:
> the
> return value can be negative or on truncation >= sizeof(buf). So the pattern
> of:
>
> n = snprintf(...);
> ttywrite(..., n, ...);
>
> Is not good.
Done, attached an amended patch. And on that topic, saw there was
another place using this pattern:
case 'n': /* DSR – Device Status Report (cursor position) */
if (csiescseq.arg[0] == 6) {
len = snprintf(buf, sizeof(buf), "\033[%i;%iR",
term.c.y+1, term.c.x+1);
ttywrite(buf, len, 0);
}
break;
Didn't touch it, since I think that's out of scope for this patch.
- NRK
>From 2ee30ead2ed4fab9625b14770c0857a8a07a7dc2 Mon Sep 17 00:00:00 2001
From: NRK
Date: Fri, 7 Jan 2022 23:21:04 +0600
Subject: [PATCH] code-golfing: cleanup osc color related code
* adds missing function prototype
* move xgetcolor() prototype to win.h (that's where all the other x.c
func prototype seems to be declared at)
* check for snprintf error/truncation
* reduces code duplication for osc 10/11/12
* unify osc_color_response() and osc4_color_response() into a single function
the latter two was suggested by Quentin Rameau in his patch review on
the hackers list.
---
st.c | 78 ++-
st.h | 2 --
win.h | 1 +
3 files changed, 25 insertions(+), 56 deletions(-)
diff --git a/st.c b/st.c
index c71fa06..12354ce 100644
--- a/st.c
+++ b/st.c
@@ -161,6 +161,7 @@ static void csidump(void);
static void csihandle(void);
static void csiparse(void);
static void csireset(void);
+static void osc_color_response(int, int, int);
static int eschandle(uchar);
static void strdump(void);
static void strhandle(void);
@@ -1843,39 +1844,25 @@ csireset(void)
}
void
-osc4_color_response(int num)
+osc_color_response(int num, int index, int is_osc4)
{
int n;
char buf[32];
unsigned char r, g, b;
- if (xgetcolor(num, , , )) {
- fprintf(stderr, "erresc: failed to fetch osc4 color %d\n", num);
+ if (xgetcolor(is_osc4 ? num : index, , , )) {
+ fprintf(stderr, "erresc: failed to fetch %s color %d\n",
+ is_osc4 ? "osc4" : "osc", is_osc4 ? num : index);
return;
}
- n = snprintf(buf, sizeof buf, "\033]4;%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
- num, r, r, g, g, b, b);
-
- ttywrite(buf, n, 1);
-}
-
-void
-osc_color_response(int index, int num)
-{
- int n;
- char buf[32];
- unsigned char r, g, b;
-
- if (xgetcolor(index, , , )) {
- fprintf(stderr, "erresc: failed to fetch osc color %d\n", index);
- return;
- }
-
- n = snprintf(buf, sizeof buf, "\033]%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
- num, r, r, g, g, b, b);
-
- ttywrite(buf, n, 1);
+ n = snprintf(buf, sizeof buf, "\033]%s%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
+ is_osc4 ? "4;" : "", num, r, r, g, g, b, b);
+ if (n < 0 || n >= sizeof(buf))
+ fprintf(stderr, "error: %s while printing %s response\n", n < 0 ?
+ "snprintf failed" : "truncation occurred", is_osc4 ? "osc4" : "osc");
+ else
+ ttywrite(buf, n, 1);
}
void
@@ -1883,6 +1870,11 @@ strhandle(void)
{
char *p = NULL, *dec;
int j, narg, par;
+ const struct { int idx; char *str; } osc_table[] = {
+ { defaultfg, "foreground" },
+ { defaultbg, "background" },
+ { defaultcs, "cursor" }
+ };
term.esc &= ~(ESC_STR_END|ESC_STR);
strparse();
@@ -1917,41 +1909,19 @@ strhandle(void)
}
return;
case 10:
- if (narg < 2)
-break;
-
- p = strescseq.args[1];
-
- if (!strcmp(p, "?"))
-osc_color_response(defaultfg, 10);
- else if (xsetcolorname(defaultfg, p))
-fprintf(stderr, "erresc: invalid foreground color: %s\n", p);
- else
-tfulldirt();
- return;
case 11:
- if (narg < 2)
-break;
-
- p = strescseq.args[1];
-
- if (!strcmp(p, "?"))
-osc_color_response(defaultbg, 11);
- else if (xsetcolorname(defaultbg, p))
-fprintf(stderr, "erresc: invalid background color: %s\n", p);
- else
-tfulldirt();
- return;
case 12:
if (narg < 2)
break;
-
p = strescseq.args[1];
+ if ((j = par - 10) < 0 || j >= LEN(osc_table))
+break; /* shouldn't be possible */
if (!strcmp(p, "?"))
-osc_color_response(defaultcs, 12);
- else if (xsetcolorname(defaultcs, p))
-fprintf(stderr, &
Re: [hackers] [dmenu|libsl][PATCH] optimize drw_text() for large strings
On Sat, Mar 19, 2022 at 11:06:25AM +0100, Stein Gunnar Bakkeby wrote: > Given that it is asking XftTextExtentsUtf8 to calculate the width of every > character every time this is checked I assume that it would be less > performant (though possibly not so easy to measure). Hi Bakkeby, You can test this patch with the following script: for _ in $(seq 20); do cat /dev/urandom | base64 | tr -d '\n' | head -c 100 echo done | ./dmenu -l 10 Try moving up and down, without the patch there should be noticeable amount of lag; with the patch it's pretty much instant for me. > Now as for the patch. It is meant as a performance improvement when strings > are too long to fit and I can see how it might be an improvement when the > string is just a little bit too long for the drawable area, but what if it > is twice as long? I think you've gotten it backwards, this patch should *faster* if the string is too long, since we're incrementing up to w instead of decerementing down it it. > In an attempt to address some of these issues I played around with moving > the XftTextExtentsUtf8 call to the section before that calculates > utf8strlen and maintaining the ew variable manually. In practice means that > the call to XftTextExtentsUtf8 is made for every single UTF-8 character > (but only one character at a time). > > In addition to this I recorded the width of "..." to calculate and keep > track of the last full UTF-8 character where the ellipsis would still fit > so that I know where to go back to should the ellipsis need to be drawn. > > I can show the implementation of that should there be any interest. Sure, I'm interested in seeing what you've come up with, especially since I've been thinking of making some changes to drw_text() myself. - NRK
Re: [hackers] [dmenu|libsl][PATCH] optimize drw_text() for large strings
Hi all,
Adding to this: I've been using this patch, both in dmenu and dwm, for
the past week or so and haven't run into any issues. If someone finds
any, feel free to send some reproduction steps.
Also noticed some small white-space issue in drw.h, don't think it's
worth sending another mail for, so I've attached the patch for it in
this mail.
- NRK
>From 69c55a03a7efe8f4157388f0ad9d02b7b97c7520 Mon Sep 17 00:00:00 2001
From: NRK
Date: Thu, 17 Mar 2022 18:24:29 +0600
Subject: [PATCH 2/3] small white space fix
---
drw.c | 2 +-
drw.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drw.c b/drw.c
index bbce179..6240865 100644
--- a/drw.c
+++ b/drw.c
@@ -166,7 +166,7 @@ xfont_free(Fnt *font)
free(font);
}
-Fnt*
+Fnt *
drw_fontset_create(Drw* drw, const char *fonts[], size_t fontcount)
{
Fnt *cur, *ret = NULL;
diff --git a/drw.h b/drw.h
index 4c67419..89872ba 100644
--- a/drw.h
+++ b/drw.h
@@ -32,8 +32,8 @@ void drw_resize(Drw *drw, unsigned int w, unsigned int h);
void drw_free(Drw *drw);
/* Fnt abstraction */
-Fnt *drw_fontset_create(Drw* drw, const char *fonts[], size_t fontcount);
-void drw_fontset_free(Fnt* set);
+Fnt *drw_fontset_create(Drw *drw, const char *fonts[], size_t fontcount);
+void drw_fontset_free(Fnt *set);
unsigned int drw_fontset_getwidth(Drw *drw, const char *text);
void drw_font_getexts(Fnt *font, const char *text, unsigned int len, unsigned int *w, unsigned int *h);
--
2.34.1
