Hacker News Reader

Comment by moomin | Hacker News Reader

The code review here is excellent and supportive. I'm very impressed.

IMO the LLVM community could learn a few things from the Rust team. I know that the activity level is not the same but it's really encouraging to new folks that reviews get engagement and disposition: "this is good, but fix that" instead of wilting without any clear indication of whether it could be merged.

Also, the fact that the CI triggers and lets you know whether your change causes any collateral damage before you merge takes away a lot of the fear factor of contributing. This is also in stark contrast to LLVM's "merge it if it LGTM, revert it soon after when we find out what it broke."

As an example of this, Zoxc has been trying to get stack probe support into LLVM for two years to no avail [0]. This is important to make sure that stack overflow or buffer overrun actually trigger the guard page and cause termination.

I admit it's not really fair to LLVM to pick on one patch like this but it's frustrating to me even as an outsider.

[0]: https://reviews.llvm.org/p/Zoxc/

Is that just a stack canary? Hasn't gcc had this for years?

No, stack canaries are different. Stack canaries defend against some classes of buffer overflows. Stack probes defend against some classes of stack overflows.

Zoxc has repeatedly refused reasonable requests to alter the patch to fit the standards of upstream. Because of this (and because I am rather tired of not having stack probes, with my embedded system not having functional guard pages), I'll make a hostile fork of their patch soon and get it through the pipeline. Stay tuned.

Ah, interesting. I hadn't gleamed that from the threads. It's great to hear that there's a chance that this will be supported though!

Late comment, but aren't stack probes entirely predicated on having working guard pages? I.e. they only exist to guarantee that the guard page is accessed before anything else, when a frame is larger than the page.

I've submitted a couple of small patches and the Rust core team has always been very helpful and pleasant.

In this case of the sort improvement, nobody in the literal Rust core team was involved in reviewing the PR.

I once submitted a patch to Cargo. Despite being uninformed and violating code style rules which I should have read and known about, they patiently guided me, I resolved the issues, and it ended up being merged. They're great folks.

The PR author's initial write-up and responses to comments were excellent too.

That struck me, too. I've found the Rust community generally very helpful, but this is really superb feedback and discussion.

Reply on news.ycombinator.com