This is an archive of the discontinued LLVM Phabricator instance.

Fix a crash bug caused by a nested call of parallelForEach.
ClosedPublic

Authored by ruiu on Apr 16 2019, 12:54 AM.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

ruiu created this revision.Apr 16 2019, 12:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2019, 12:54 AM
Herald added a subscriber: aheejin. · View Herald Transcript
andrewrk accepted this revision.Apr 16 2019, 12:55 AM

I tested this and confirm that it solves the bug (which I reported).

This revision is now accepted and ready to land.Apr 16 2019, 12:55 AM
ruiu added a comment.Apr 16 2019, 1:06 AM

I will keep this patch for a day to give sbc100 to take a look.

This patch should be included in 8.0.1 once landed.

sbc100 accepted this revision.Apr 16 2019, 11:08 AM

So IIUC, this moves the parallelism to per-input-file, rather per-chuck-per-input-file.

@ruiu presumably we want to restore more parallelism if we land this change? Having said that perhaps this won't be much of regression for reasonable sized projects since the projects that benefit from high parallelism will most likely have many input files. One concern is LTO, where there can be essentially one giant object file, in which case I think we would loose all our parallelism. However I imagine in that case that the link time would massively dominated by the compile time?

Otherwise LGTM.

Also, it makes me sad that parallelForEach can't detect this behaviour, especially since it leads to bugs that are hard to reliably reproduce. Should we try to get parallelForEach fixed?

So IIUC, this moves the parallelism to per-input-file, rather per-chuck-per-input-file.

@ruiu presumably we want to restore more parallelism if we land this change? Having said that perhaps this won't be much of regression for reasonable sized projects since the projects that benefit from high parallelism will most likely have many input files. One concern is LTO, where there can be essentially one giant object file, in which case I think we would loose all our parallelism. However I imagine in that case that the link time would massively dominated by the compile time?

Otherwise LGTM.

Also, it makes me sad that parallelForEach can't detect this behaviour, especially since it leads to bugs that are hard to reliably reproduce. Should we try to get parallelForEach fixed?

Sorry, I just saw https://reviews.llvm.org/D60758. lgtm

ruiu added a comment.Apr 16 2019, 7:03 PM

There should not be a fundamental reason for parallelForEach to not be reentrant, so we eventually want to fix it so that we can call the function without worrying the nested condition. For the time being, we should be able to detect the error condition more easily by adding the assertions.

This revision was automatically updated to reflect the committed changes.