Allow sbrk on arm64 and riscv
AcceptedPublic
Actions

Authored by kib on Nov 30 2023, 7:40 PM.

Details

Reviewers

emaste
alc
markj
andrew
brooks

Summary

break(2): do execute everywhere

stop pretending that arm64 and riscv cannot provide the syscall.  So far
it only annoyed porters.

libc: export brk and sbrk symbols on all architectures

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kib created this revision.Nov 30 2023, 7:40 PM

Herald added a subscriber: imp. · View Herald TranscriptNov 30 2023, 7:40 PM

kib requested review of this revision.Nov 30 2023, 7:40 PM

I'm glad that commit 9f9c9b22ecfd9e87cbe9e1becf946faeb5fbcac6 allowed some time to be saved here. :)

lib/libc/tests/sys/Makefile should be updated too, so that brk_test is installed unconditionally. (I verified that those tests pass on arm64 with this patch applied.)

In D42853#977452, @markj wrote:

I'm glad that commit 9f9c9b22ecfd9e87cbe9e1becf946faeb5fbcac6 allowed some time to be saved here. :)

Yes, that commit was of course the main bits to allow this. But I do not see why it was not finished then.

Also install test.

In D42853#977457, @kib wrote:

In D42853#977452, @markj wrote:

I'm glad that commit 9f9c9b22ecfd9e87cbe9e1becf946faeb5fbcac6 allowed some time to be saved here. :)

Yes, that commit was of course the main bits to allow this. But I do not see why it was not finished then.

I'm not sure what you mean - what was not finished? At the time of that commit, arm64 and riscv already had removed break(2) support.

This revision is now accepted and ready to land.Nov 30 2023, 9:07 PM

emaste accepted this revision.Nov 30 2023, 9:14 PM

In D42853#977466, @markj wrote:

In D42853#977457, @kib wrote:

In D42853#977452, @markj wrote:

I'm glad that commit 9f9c9b22ecfd9e87cbe9e1becf946faeb5fbcac6 allowed some time to be saved here. :)

Yes, that commit was of course the main bits to allow this. But I do not see why it was not finished then.

I'm not sure what you mean - what was not finished? At the time of that commit, arm64 and riscv already had removed break(2) support.

I mean that (re)enabling break() on the arches should be right after the commit.

I won't attempt to block this change, but I don't like it. break/sbrk is an awful interface and can't be fixed. IMO we're better off without it.

Which ports (or possible ports) are having issues because of a lack of sbrk?

I know emacs had issues in the past, however our lack of sbrk helped them finally fix their broken use of it (and fix emacs when aslr is enabled). Recently vlang [1] was mentioned, however it appears to be an easy fix to switch to mmap as the mmap support is already there, it's just not enabled by default.

[1] https://github.com/vlang/v

In D42853#977549, @andrew wrote:

Which ports (or possible ports) are having issues because of a lack of sbrk?

I know emacs had issues in the past, however our lack of sbrk helped them finally fix their broken use of it (and fix emacs when aslr is enabled). Recently vlang [1] was mentioned, however it appears to be an easy fix to switch to mmap as the mmap support is already there, it's just not enabled by default.

[1] https://github.com/vlang/v

I do not remember exactly, just that the mentioning of sbrk(2) as an annoying missed feature happens.

I do not see a point keeping the interface disabled while it is implemented.

D42862 is my preferred alternative (plus a patch I've not published to fix the tests)

alc accepted this revision.Dec 1 2023, 8:27 AM

I *will* object to this. We've had multiple releases without this syscall. sbrk is an absolutely awful interface in this day and age. Eradicating its use is a worthwhile goal, and the primary culprits have been fixed. As time goes on it will become even less relevant. It seems wrong to go and add it back now for the minuscule set of irrelevant packages that still believe it's 1989 and risk new bogus uses of the functions creeping in. We should be striving to remove it from all architectures (with ABI compat for old releases), not adding it in more places.

That is, I believe @brooks's approach is the one we should be striving for, i.e. taking steps forwards and eradicating it in 15, not going backwards like this patch proposes.