Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Fix Issue 19593 - use memcmp instead of dstrcmp #2467

Merged
merged 1 commit into from Jan 19, 2019

Conversation

kubo39
Copy link
Contributor

@kubo39 kubo39 commented Jan 18, 2019

Using dstrcmp inside trace_addsym causes stack overflow, because dmd
inserts trace_pro function to top of dstrcmp when -profile option is added.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @kubo39! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
19593 minor dstrcmp with -profile causes stack overflow

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2467"

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Jan 18, 2019
Copy link
Member

@rainers rainers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you are not recompiling the runtime with -profile, but that the issue is that rt.trace and your code reference the same symbol dstrcmp, but also generate different implementations. The linker picks the one with profiling hooks causing the stack overflow in the profiler code.

Please also remove the respective import.

@rainers
Copy link
Member

rainers commented Jan 19, 2019

There seem to be other changes to the profile.log, too:

diff mytrace.def.exp ./generated/linux/debug/64/mytrace.def
3c3
< _D4core8internal6string__T7dstrcmpZQjFNaNbNiNeMxAaMxQeZi
---
> _D6object__T5__cmpTaZQjFNaNbNiNeMxAaMxQeZi
4a5
> _D4core8internal6string__T7dstrcmpZQjFNaNbNiNeMxAaMxQeZi
Makefile:13: recipe for target 'generated/linux/debug/64/profile.done' failed```

@kubo39
Copy link
Contributor Author

kubo39 commented Jan 19, 2019

I assume you are not recompiling the runtime with -profile, but that the issue is that rt.trace and your code reference the same symbol dstrcmp, but also generate different implementations. The linker picks the one with profiling hooks causing the stack overflow in the profiler code.

Ah, yes, I found this at FuncDeclaration__toObjFile in dmd/src/dmd/glue.d (sorry for my poor english..)

Using dstrcmp inside trace_addsym causes stack overflow, because dmd
 inserts trace_pro function to top of dstrcmp when -profile is added.
Copy link
Member

@n8sh n8sh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dlang-bot dlang-bot merged commit 5960787 into dlang:master Jan 19, 2019
@kubo39 kubo39 deleted the fix-issue19593 branch January 19, 2019 18:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Fix Include reference to corresponding bugzilla issue
Projects
None yet
4 participants