In D61126#1402476, @doonbsd_gmail.com wrote:

In D61126#1402217, @jrtc27 wrote:

Can you please (a) avoid unnecessary churn (b) separate out and properly justify each logical change? The a1 addition in vmm_sbi_ecall seems entirely unrelated, but I also don't understand why it's needed, so if you believe it is, put it in its own revision with a proper commit message.

(b) is required as per specification. SBI call returns sbiret.{error, value}. Error is returned in a0 and value in a1. As per SBI specification, if a function has no return value, a1 must be 0. This path is being SBI spec compliant. Does it hurt here? or is this change big enough to stand alone in a separate patch?

Should we move the random instance inside the softc and make it work instead? Also means only paying for the space if you need it, not that it’s big.

Can you please (a) avoid unnecessary churn (b) separate out and properly justify each logical change? The a1 addition in vmm_sbi_ecall seems entirely unrelated, but I also don't understand why it's needed, so if you believe it is, put it in its own revision with a proper commit message.

That is not in general sufficient reasoning

Commit message does not say the "why", only the "what"; i.e. it doesn't say under what circumstances this can happen. Can it happen? It would seem like a rather broken state to be in.

In D59710#1386475, @code_fedang.net wrote:

In D59710#1385456, @jrtc27 wrote:

Alternatively, why not just adjust the existing loop that populates segs to instead take min(p_vaddr) and max(p_vaddr + p_memsz)? It wouldn't be hard to do and would remove the need for this restriction (and without any complex sorting). The only thing you'd miss is handling overlapping segments, but to be honest I don't think that would necessarily be a problem, we'd do exactly what the file asked for and load the segments in that order, but it just might result in the module shooting itself in the foot, which I don't see as any different from the countless other ways a module can corrupt its own internal state and panic your kernel. At the end of the day if you're loading a module into the kernel you're trusting the module to not itself be a panic-fest, all the kernel linker should be doing is avoiding confusing/corrupting things *outside* the module.

I thought about that at the start, but ended up with the extra check since the subsequent code seems to rely on that.
Overall, I think a sane policy for malformed modules would be: if it has to crash, it should do so outside of the linker.

Alternatively, why not just adjust the existing loop that populates segs to instead take min(p_vaddr) and max(p_vaddr + p_memsz)? It wouldn't be hard to do and would remove the need for this restriction (and without any complex sorting). The only thing you'd miss is handling overlapping segments, but to be honest I don't think that would necessarily be a problem, we'd do exactly what the file asked for and load the segments in that order, but it just might result in the module shooting itself in the foot, which I don't see as any different from the countless other ways a module can corrupt its own internal state and panic your kernel. At the end of the day if you're loading a module into the kernel you're trusting the module to not itself be a panic-fest, all the kernel linker should be doing is avoiding confusing/corrupting things *outside* the module.

If you’re to use them, at least have the decency to remove all the slop and write a proper commit message. As it stands this is a very low-effort change that feels extractive.

Did you use LLMs to generate this?

The only reason to do have this singular RWX segment would be if whatever's loading the kernel cannot tolerate more than one PT_LOAD. I hope that's really not the case. Manually specifying the program headers really sucks as a thing to do (no PT_NOTE for NT_GNU_BUILD_ID, no PT_GNU_RELRO, no PT_INTERP with /red/herring, and any other helpful things your toolchain might normally add...).

I intend to review this in the coming days

Please go read all of D40474

A higher level comment: I don't think we should be landing tree-wide changes to support -fbounds-safety unless there's community buy-in for actually adopting it as an in-tree option in parts of the kernel. Otherwise we're picking up a lot of churn and annotations that will bit-rot and pollute the code for no gain.

Prevailing style is to put a space between the * and qualifier that follows, which all these new annotations should follow.

Is this necessary? Do you not want LLDB to print trapframes as special, like KGDB does, and like GDB does for signal frames?

How did a plugin end up with a broken ELF note, and why are we shipping it?

In D59689#1369873, @mchoo wrote:

ping @jhb for mentor approval

Um, yeah, if pkg produces a METALOG that is not readable by makefs, due to not escaping the whitespace, that's a bug in pkg to fix in pkg? And it's impossible to reliably escape at this point, because it is inherently ambiguous whether what comes after the space is a continuation of the path or a new key=value pair. So we really should not be trying to guesstimate what the correct METALOG entry is.

In D59269#1360942, @kib wrote:

In D59269#1360939, @jrtc27 wrote:

Regarding leaking, glibc does seem to store them in a hash table tied to link_map (i.e. Obj_Entry) so will free on dlclose, unlike our arm64 implementation. Linked list seems a bit unnecessarily slow, but at the same time extensive dynamic TLSDESC + dlclose is probably not common enough to actually benefit from something like a reallocarray-based storage.

The list is only traversed on dlclose() to free items, so I do not see it as causing any slowdown. In fact, where the hashed lookup would be useful?

Regarding leaking, glibc does seem to store them in a hash table tied to link_map (i.e. Obj_Entry) so will free on dlclose, unlike our arm64 implementation. Linked list seems a bit unnecessarily slow, but at the same time extensive dynamic TLSDESC + dlclose is probably not common enough to actually benefit from something like a reallocarray-based storage.

Looking at the glibc source, they do have an equivalent to the dtv_gen field, but the fast path check is whether the DTV's generation is >= that field, not ==. I think this is a time optimisation that avoids the need to access the global generation in order to do the check, instead just using a copy that's more to hand, and also lets you avoid taking the slow path for DTV entries that don't actually need it, even though the generation counter is ahead (e.g. if you dlopen A, use TLS for A, dlopen B, then use TLS for A again, you can still use the fast path, but ye olde __tls_get_addr doesn't know that, so will pessimistically update the DTV for B's load).

I believe both to be true, yes (and have previously deduced the latter myself, too...)

Minor nits on the implementation but looks functionally correct. Will defer to Alex and others for finessing the manpage.

IMO it's fine to mix abbreviated and full forms of the same term (and I guess the mix-and-match "debugging info" and "debug information"); you wouldn't want the long form here as the dialog is quite small, but that's fine in prosaic text. But yes, the "data" ones should be converted over. Sometimes we also say "files", but that's also fine as it's talking about the files themselves not their contents (though one might argue they should be "debug(ging) info(rmation) files").

-g, -gline-tables-only, -gmodules

	    Control debug information output

But LGTM aside from that

Thanks

Looks like phab mangled my tabs so the updated lines are all using spaces :(

I just used their email address for Reported by because given (a) the lack of history with the project (b) no trace of them existing anywhere else online (c) the low-quality suspicious nature of the patches (d) the name being one of *the* quintessential placeholders, I am suspicious as to whether it's their real name.

There's also lib/libc/gen/tls.c for PT_TLS

Fixed properly in 1e39a314d870e312f623199e146eda6bdbc293a3.

I don't think you tested this.

D58301 seems a better alternative to this?

In D58301#1337950, @aokblast wrote:

In D58301#1337946, @jrtc27 wrote:

In D58301#1337933, @aokblast wrote:

In D58301#1337930, @jrtc27 wrote:

In D58301#1337928, @aokblast wrote:

In D58301#1337926, @jrtc27 wrote:

bus_read_8 has been replace to bus_space_read series in sys/bus.h.

Not sure what "bus_space_read series" is though, should that not be bus_space_read_8?

Sorry, it should be bus_space_{read,write} series.

But why series? I don't understand. Why not just use the _8 suffix everywhere?

The _8 suffix remains unchanged before and after my patch. The purpose of this change is to replace the original bus_ series with the bus_space_ series.

Ok, I think I now can decipher what you're trying to get at. What you're trying to say, I think, is something like the following?

bus_{read,write}_8 are macro wrappers around the corresponding bus_space functions in sys/bus.h, so implementing bus_{read,write}_8 won't work. Implement the underlying bus_space functions instead.

Yes, sorry about my bad english expression:)

In D58301#1337933, @aokblast wrote:

In D58301#1337930, @jrtc27 wrote:

In D58301#1337928, @aokblast wrote:

In D58301#1337926, @jrtc27 wrote:

bus_read_8 has been replace to bus_space_read series in sys/bus.h.

Not sure what "bus_space_read series" is though, should that not be bus_space_read_8?

Sorry, it should be bus_space_{read,write} series.

But why series? I don't understand. Why not just use the _8 suffix everywhere?

The _8 suffix remains unchanged before and after my patch. The purpose of this change is to replace the original bus_ series with the bus_space_ series.

In D58301#1337928, @aokblast wrote:

In D58301#1337926, @jrtc27 wrote:

bus_read_8 has been replace to bus_space_read series in sys/bus.h.

Not sure what "bus_space_read series" is though, should that not be bus_space_read_8?

Sorry, it should be bus_space_{read,write} series.

bus_read_8 has been replace to bus_space_read series in sys/bus.h.

Huh, I hadn't thought of doing it this way. This does seem like a better approach than D58318 and, assuming there's no concern about other code accidentally using these (which is no different to the intent of the existing bus_read_8/bus_write_8 stubbing), reintroducing the READ8/WRITE8 macros (whether called that or something else).

(But also happy with the current diff if you'd rather go with that)