Fix Issue 17898 - Segfault in compile with -deps and -debug/unittest #9122
Conversation
MartinNowak
requested review from
andralex,
CyberShadow,
Geod24,
ibuclaw,
jacob-carlborg,
dnadlinger,
rainers,
WalterBright and
wilzbach
as code owners
|
Thanks for your pull request, @MartinNowak! Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "stable + dmd#9122" |
Revert "Merge pull request dlang#6748 from RazvanN7/Fix_Issue_7016" This reverts commit afebe0c, reversing changes made to a91bc97. The original change was apparently never used for rdmd and made `-deps` unusable due to causing segfault during IR-gen. Furthermore the PR killed a common use-case for the `-deps` switch, collecting shallow dependencies for make et.al. And lastly https://dlang.org/changelog/2.079.0.html#includeimports added an alternative implementation for recursive compilation, so recursive dependencies are no longer needed for rdmd. I'm also not aware of any other user of recursive dependencies.
MartinNowak
force-pushed
the
fix17898
branch
from
43cb8fe
to
5650860
Compare
|
sociomantic codes uses it. It's not a the majority of code but there is a usecase. |
- based on parallel single file compilation - writes object files to local .dub/ninja/<package> folder (use `ninja -t clean` to cleanup) - different build types require regeneration of .ninja files (see https://ninja-build.org/manual.html#_philosophical_overview) - requires dlang/dmd#9122 for non-segfaulting `-deps`
|
cc @RazvanN7 |
Could you please elaborate a bit on this @UplinkCoder. What build-tool do you use that requires recursive dependencies from a single entry point? Could you switch to a lazy per-object dependency file or compile all sources instead? How do you avoid compiler segfaults for debug and unittest builds with NB: It seems that surrounding this change 2 separate use-cases got often conflated. The build models of rdmd (single entry point source, trying to get recursive dependencies for the actual compile) and make et.al. (generating shallow dependencies during compilation to later trigger incremental rebuilds). |
|
Maybe after all we need a new switch for recursive dependencies? Both use cases are valid and the segfault in IR is most likely due to an enormous dependency chain. |
|
We don't use phobos, we use ocean and that works fine. our build system it called |
|
I don't understand Sociomatic's needs here. Do they want this PR, or does this PR break their code? |
|
This PR could break our code. I am on holiday right now, and so are most of our developers. |
|
One point here is that recursive dependencies never worked together with codegen except for trivial examples, as semantic3 order issues trigger bugs in IR and codegen. Apparently Sociomantic only uses Dependencies for their build targets rely on rdmd's |
It's rather due to the fragile backend/IR-gen that cannot deal with semantic3 calls on arbitrary modules which might not even be part of the compilation. |
Ping @UplinkCoder |
Ping @UplinkCoder ;-) |
Codecov Report
@@ Coverage Diff @@
## stable #9122 +/- ##
=========================================
- Coverage 85.8% 85.8% -0.01%
=========================================
Files 141 141
Lines 72285 72276 -9
=========================================
- Hits 62026 62018 -8
+ Misses 10259 10258 -1
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## stable #9122 +/- ##
=========================================
- Coverage 85.8% 85.8% -0.01%
=========================================
Files 141 141
Lines 72285 72276 -9
=========================================
- Hits 62026 62018 -8
+ Misses 10259 10258 -1
Continue to review full report at Codecov.
|
|
How come Sociomantic uses this @UplinkCoder ? This was introduced in 2.075, which Sociomantic didn't use at the time. |
|
I fixed the merge conflicts. |
|
@UplinkCoder ping? |
|
@Geod24 I've back-ported this to dmd-transitional I can't recall which code only that it was If you think making the the |
|
I agree that the original fix is broken and needs to be fixed somehow, but simply reverting it is not a solution. As I see it, if you want the full dependencies that would require to do all the semantic passes (in order to fix the existing breakages) and that would make deps unusable in certain situations where only the shallow dependencies are needed. The solution I am proposing is the following: make the -deps switch receive a string parameter that may have the values I don't see any other way of solving this conflict, since there are parties that are interested in getting the recursive dependencies. |
This currently creates a file "shallow". If the current behavior is reverted, you should get the recursive dependencies by combining -deps and -i. |
|
Breaking sociomantic code is no longer a blocker. The comment on Can the tests be kept, but the compile args fixed up accordingly? |
Revert "Merge pull request #6748 from RazvanN7/Fix_Issue_7016"
This reverts commit afebe0c, reversing
changes made to a91bc97.
The original change was apparently never used for rdmd and made
-depsunusabledue to causing segfaults during IR-gen. Furthermore the PR killed a common
use-case for the
-depsswitch, collecting shallow dependencies for make et.al.And lastly https://dlang.org/changelog/2.079.0.html#includeimports added an
alternative implementation for recursive compilation, so recursive dependencies
are no longer needed for rdmd. I'm also not aware of any other user of
recursive dependencies.