Page MenuHomeFreeBSD

rpcgen: make compiler arglist allocation dynamic
ClosedPublic

Authored by brooks on Oct 10 2019, 4:36 PM.

Details

Summary

In CheriBSD we exceed the 19 non-NULL arguments in the static array. Add
a simple size doubling allocator and increase the default to 32.

GC remnants of support for fixed arguments.

Test Plan

Survives a buildworld including some calls to rpcgen.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

brooks created this revision.Oct 10 2019, 4:36 PM
arichardson accepted this revision.Oct 10 2019, 9:03 PM

I wish the C standard library had some kind of dynamic array so that this is not reimplement in every program...

I wonder if we could use something like the utarray.h macros from uthash?
Could we import the five headers that https://github.com/troydhanson/uthash into contrib and use utarray instead? I know the library is not that efficient since it uses strdup() for all strings that you insert but at least it means not reinventing the wheel.
We already have two copies of uthash.h in the source tree and elftoolchain also includes with utarray.h.

But for now this definitely looks good to me.

This revision is now accepted and ready to land.Oct 10 2019, 9:03 PM

I wish the C standard library had some kind of dynamic array so that this is not reimplement in every program...
I wonder if we could use something like the utarray.h macros from uthash?
Could we import the five headers that https://github.com/troydhanson/uthash into contrib and use utarray instead? I know the library is not that efficient since it uses strdup() for all strings that you insert but at least it means not reinventing the wheel.
We already have two copies of uthash.h in the source tree and elftoolchain also includes with utarray.h.

We certainly could use utarray instead, though I'm not super convinced by its readability.

(Importing uthash.h into contrib is on my todo list since I'd like to merge my changes to use it instead of bdb for hashes in init and a couple other places.)

I wish the C standard library had some kind of dynamic array so that this is not reimplement in every program...
I wonder if we could use something like the utarray.h macros from uthash?
Could we import the five headers that https://github.com/troydhanson/uthash into contrib and use utarray instead? I know the library is not that efficient since it uses strdup() for all strings that you insert but at least it means not reinventing the wheel.
We already have two copies of uthash.h in the source tree and elftoolchain also includes with utarray.h.

We certainly could use utarray instead, though I'm not super convinced by its readability.
(Importing uthash.h into contrib is on my todo list since I'd like to merge my changes to use it instead of bdb for hashes in init and a couple other places.)

Sounds good. I agree that utarray is quite ugly (and inefficient) but we could still use it to avoid reimplementing dynamic vectors for non-performance-critical code duplication. I haven't looked if there are any alternatives, I only discovered it because we already use uthash.

jrtc27_jrtc27.com added inline comments.
usr.bin/rpcgen/rpc_main.c
924 ↗(On Diff #63128)

This is undefined behaviour the first time round as arglist is NULL, even though argcount is 0. Also, a realloc+memset should be faster than calloc+memcpy, whilst also side-stepping this issue (and without having to introduce a branch on arglist).

usr.bin/rpcgen/rpc_main.c
924 ↗(On Diff #63128)

In fact we don't even need the memset; addarg((char *)NULL); is always called just before the execvp so the array will always be NULL-terminated.

brooks updated this revision to Diff 63268.Oct 14 2019, 6:46 PM
  • Use realloc() rather than calloc()+memcpy().
  • Limit argmax to prevent overflow (no possible on FreeBSD due to ARG_MAX).
This revision now requires review to proceed.Oct 14 2019, 6:46 PM
brooks marked 2 inline comments as done.Oct 14 2019, 10:21 PM
This revision was not accepted when it landed; it landed in state Needs Review.Oct 15 2019, 4:05 PM
This revision was automatically updated to reflect the committed changes.