Check for mmap failure with MAP_FAILED, not NULL

[mmcco]

From upstream. Here's the commit:

gcc-mirror/gcc@37bab84

And the associated PR:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67457

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[mmcco]

MAP_FAILED is typically (void *)-1, so the current code will segfault if mmap fails. This is a reasonable possibility because out-of-memory conditions cause Rust programs to fail.

[nagisa]

Maybe we should just update our libbacktrace to one which contains that commit?

[mmcco]

It seems like a good bit has changed, including the addition of things like PE/COFF support that we probably don't need. I can look more at the amount of divergence, but it may be easier to cherrypick bugfixes and leave the rest.

[sfackler]

How much has our libbacktrace diverged from upstream? It seems like we'd ideally be pulling it in as a submodule like we do for llvm etc.

[mmcco]

I'm an outsider so my opinion doesn't matter much here, but I don't see a reason to create another submodule. Honestly, I don't see why they're used at all. They're a complication that makes it harder to submit patches and keep local clones reliably synced.

[retep998]

including the addition of things like PE/COFF support that we probably don't need.

We do use libbacktrace for -gnu targets on Windows still, so it would be nice to have them.

[nagisa]

I don't see why they're used at all. They're a complication that makes it harder to submit patches and keep local clones reliably synced.

IMO advantages of submodules strongly outweight their disadvantages. Namely, they allow updating and upstreaming your own patches easier. It doesn’t infest history of the project with history of other pojects etc.

I'm an outsider so my opinion doesn't matter much here

You’re contributing now, so it does indeed matter.

[mmcco]

It looks like I should have spent more time with the code rather than talking so much. :-)

As it turns out, we last pulled libbacktrace in August. I'm not sure why I thought otherwise. There have been only a few upstream changes since August (and one of them seems to just add a completely unnecessary type cast).

The last two merges were done by @Diggsey and @timbertson. This just involved importing the code and reapplying two patches: one for Bitrig support and one for OpenBSD support.

However, it seems that @Diggsey and @Aarzee have made more local changes since then. Some of these just involve style. It's probably worth abandoning those so that we can stay closer to upstream. Others involve simplification and bug-fixing, which seems nice. We should probably submit them upstream if we haven't already.

In terms of using a submodule, remember that libbacktrace is SVN-hosted and is a subdirectory of GCC. I don't know how much harder this makes it for us to fork a submodule.

[nagisa]

In terms of using a submodule, remember that libbacktrace is SVN-hosted and is a subdirectory of GCC. I don't know how much harder this makes it for us to fork a submodule.

There’s https://github.com/gcc-mirror/gcc

[mmcco]

@nagisa As long as the mirror's sufficiently reliable and frequently updated, I don't know why it wouldn't work. I'll stay out of the submodule thing, as I'm slightly opposed to it and don't feel like getting involved.

I'm going to close this ticket and continue working on a full pull from upstream. I think I'm going to start with a PR backing out @Aarzee's whitespace tweaks, as they'll just be a never-ending merge headache until we do. Hopefully someone with commit rights can fast-track that for me so we can keep this rolling.

[mmcco]

See #30687

[Diggsey]

@mmcco FWIW, I think you've got the right idea here. I hadn't intended to modify libbacktrace after updating, other than re-applying @timbertson's patch. The change was a debugging aid I had to add while tracking down a bug in buildbot, and it seems the PR went through without removing it.

Using a submodule, even if it's a custom branch, would be good. The main issue is that libbacktrace is a subfolder in the gcc repo, so even though the gcc mirror exists, afaik it's not possible to add a subfolder of a git repo as a submodule (this is something I looked into when updating libbacktrace).

IMO, the best option is to keep our own github mirror of just the libbacktrace subfolder, and then fork that to add any custom changes. We can add this fork as a submodule and everything should work.

[sfackler]

git subtree may be a better option than a submodule given that we only want a certain directory out of the repo: http://blogs.atlassian.com/2013/05/alternatives-to-git-submodule-git-subtree/

[Diggsey]

@sfackler subtree is interesting, but one of the downsides mentioned in the article may be a problem: it doesn't prevent accidental modification of the subtree in a commit, and more than that, AFAICT there's no obvious indication to future committers that the libbacktrace folder is anything other than a normal folder.

It's still an improvement over the current situation though!

[mmcco]

To me, this all sounds like overkill for a 5.1 KLoC single-directory inclusion. I'm happy to be the person who spends five minutes manually reapplying patches every few months in order to avoid this added complexity.

[mmcco]

AFAICT there's no obvious indication to future committers that the libbacktrace folder is anything other than a normal folder.

It's all in C and has its own build infrastructure, which immediately tipped me off. Also, people who are familiar with the codebase will probably be aware that it's a third-party inclusion.

[alexcrichton]

Thanks for this @mmcco! FWIW we have no local changes to libbacktrace, as mentioned here we don't use a submodule as the "official" location as far as I know is in the gcc repo, and we probably don't want a submodule to all of gcc!

I would be fine just copying in all the files from the gcc repo to update what we've got locally, it doesn't matter much if there's conflicts or such (we just want to always prefer what's upstream).

[mmcco]

@alexcrichton:

FWIW we have no local changes to libbacktrace

We have at least a few:

• a tweak for OpenBSD support
• a similar tweak for Bitrig support
• adding line numbers to Windows-GNU backtraces
• a bunch of whitespace tweaks that we're backing out of in Undo whitespace tweaks pending libbacktrace merge #30687

I also just realized that we may have another set of whitespace tweaks to back out of... (commits cd8f317 and d51047d)... I can look into that.

Thanks!
Mike

[alexcrichton]

Ah yeah that's a little more extensive than I thought, but fundamentally there are no major changes, it's just a few tweaks here and there which can all be pretty easily reapplied (or done after an update)

[mmcco]

Agreed.

[petrochenkov]

@mmcco
Are you planning to update libbacktrace in the near future?
I'm going to investigate #28447, but I'd prefer to have libbacktace updated before starting.

[mmcco]

@petrochenkov Yup, I was planning on getting to it tomorrow. Do you already have a PR open?

[petrochenkov]

@mmcco

Do you already have a PR open?

Yes. (Sorry for intervening.)

Also, our libbacktrace is not only gcc/libbacktrace subdirectory, but also several random headers and build scripts from gcc and gcc/include, so submodules/subtrees were probably not a choice.

[mmcco]

@petrochenkov Works for me. :) Thanks!