Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix InvalidMemoryOperationError in DebugAllocator #848

Merged
merged 1 commit into from
Oct 14, 2014
Merged

Fix InvalidMemoryOperationError in DebugAllocator #848

merged 1 commit into from
Oct 14, 2014

Conversation

etcimon
Copy link
Contributor

@etcimon etcimon commented Oct 2, 2014

This fixes errors which fail unit tests when using DebugAllocator.

@s-ludwig
Copy link
Member

s-ludwig commented Oct 2, 2014

Hm, but like this it doesn't make sense. It now tries to remove a "pointer to a pointer to void" from a set of "pointers to void". Do you have a call stack for the invalid memory operation error?

@etcimon
Copy link
Contributor Author

etcimon commented Oct 2, 2014

I couldn't get a call stack no, but I traced it using log trace. It occurs somewhere around http/router.d:320

@s-ludwig
Copy link
Member

s-ludwig commented Oct 2, 2014

I'll see if I can reproduce it. But probably I won't be able to make it before Saturday, as I need to prepare for a journey.

@etcimon
Copy link
Contributor Author

etcimon commented Oct 2, 2014

Oh, just uncomment memory.d:46 and run dub test. That's alright, it's only annoying not knowing where it comes from the first time it happens. You're right though, my fix only works because it doesn't remove anything. Btw, the pointer it tries to remove looks really odd

@etcimon
Copy link
Contributor Author

etcimon commented Oct 2, 2014

Actually not that odd, it's 23C1F00 and 23D6550, using try/catch does the thing on linux.

@etcimon
Copy link
Contributor Author

etcimon commented Oct 2, 2014

I figured out a way to get a stack trace (calling kill(getpid(), SIGTERM) in the catch clause to cause a break in GDB):

Program received signal SIGTERM, Terminated.
[Switching to LWP 16187]
0x00007ffff6cea737 in kill () at ../sysdeps/unix/syscall-template.S:81
81 T_PSEUDO (SYSCALL_SYMBOL, SYSCALL_NAME, SYSCALL_NARGS)
(gdb) backtrace
#0 0x00007ffff6cea737 in kill () at ../sysdeps/unix/syscall-template.S:81
#1 0x00000000006a27c3 in libasync.internals.memory.DebugAllocator.free() (this=0x7ffff7ed9500, mem=...)
at ../libasync/source/libasync/internals/memory.d:185
#2 0x00000000006a1ff2 in libasync.internals.memory.LockAllocator.free() (this=0x7ffff7ed8b80, mem=...)
at ../libasync/source/libasync/internals/memory.d:127
#3 0x00000000005e83cc in libasync.internals.memory.__T11FreeListRefTC4vibe6stream8counting20CountingOutputStreamVbi1Z.FreeListRef.clear() (this=0x7ffff7efc578) at ../libasync/source/libasync/internals/memory.d:662
#4 0x00000000005e8215 in libasync.internals.memory.__T11FreeListRefTC4vibe6stream8counting20CountingOutputStreamVbi1Z.FreeListRef.__dtor() (this=0x7ffff7efc578) at ../libasync/source/libasync/internals/memory.d:621
#5 0x00000000005e8ddc in vibe.http.server.HTTPServerResponse.__fieldDtor() (this=0x7ffff7efc000)
at source/vibe/http/server.d:735
#6 0x00000000006e0044 in rt_finalize2 ()
#7 0x000000000073d54f in gc.gc.Gcx.fullcollect() ()
#8 0x000000000073a082 in gc.gc.GC.mallocNoSync() ()
#9 0x0000000000739f82 in gc.gc.GC.malloc() ()
#10 0x00000000006dce05 in gc_malloc ()
#11 0x00000000006d8cfc in core.memory.GC.malloc() ()
#12 0x00000000006a2bb1 in libasync.internals.memory.GCAllocator.alloc() (this=0x7ffff7f7a1a0, sz=64)
at /usr/src/d/install/src/druntime/import/core/memory.d:338
#13 0x00000000006a471f in libasync.internals.memory.FreeListAlloc.alloc() (this=0x7ffff7f7c100)
at ../libasync/source/libasync/internals/memory.d:518
#14 0x00000000006a3259 in libasync.internals.memory.AutoFreeListAllocator.alloc() (this=0x7ffff7f99c00, sz=64)
at ../libasync/source/libasync/internals/memory.d:291
#15 0x00000000006a22b5 in libasync.internals.memory.DebugAllocator.alloc() (this=0x7ffff7f9adc0, sz=64)
at ../libasync/source/libasync/internals/memory.d:149
#16 0x00000000006a1d5e in libasync.internals.memory.LockAllocator.alloc() (this=0x7ffff7f7a180, sz=64)
at ../libasync/source/libasync/internals/memory.d:115
#17 0x0000000000562ed8 in vibe.utils.array.__T13AllocAppenderTAhThZ.AllocAppender.reserve() (this=0x7fffffffe018,
amt=64) at source/vibe/utils/array.d:74
#18 0x000000000061292c in vibe.stream.memory.MemoryOutputStream.reserve() (this=0x7fffffffe008, nbytes=64)
at source/vibe/stream/memory.d:45
#19 0x000000000061a2d5 in vibe.stream.operations.__T9readUntilZ.readUntil() (alloc=0x7ffff7f7a198,
max_bytes=18446744073709551615, end_marker=..., stream=0x7ffff7fa2a40) at source/vibe/stream/operations.d:92
#20 0x0000000000619b23 in vibe.stream.operations.__unittestL206_239() (this=0x7fffffffe1a0, expected=0, s=...)
at source/vibe/stream/operations.d:89
#21 0x000000000061997a in vibe.stream.operations.__unittestL206_239() () at source/vibe/stream/operations.d:220
#22 0x000000000061abd1 in vibe.stream.operations.__modtest() ()
#23 0x000000000072926c in core.runtime.runModuleUnitTests() ()

@etcimon
Copy link
Contributor Author

etcimon commented Oct 2, 2014

Oh, I debugged this already for LDC. I thought it was compiler specific:

https://github.com/etcimon/libasync/blob/master/source/libasync/internals/memory.d#L654

@etcimon
Copy link
Contributor Author

etcimon commented Oct 2, 2014

Now I'm worried that it may leave entries in DebugAllocator. I'll get back to it later

@s-ludwig
Copy link
Member

s-ludwig commented Oct 2, 2014

Ah okay... so it's because the HTTPServerResponse contains FreeListRef and is allocated on the heap. So either the test response would have to be returned as a FreeListRef, too, or DebugAllocator would have to use a custom type for m_blocks that doesn't invoke the GC when removing an entry.

@etcimon
Copy link
Contributor Author

etcimon commented Oct 2, 2014

Oh, that's a great solution. Plus, I've gotten used to using the HashMap type as an indirection =)

s-ludwig
s-ludwig reviewed Oct 7, 2014
View reviewed changes
source/vibe/utils/memory.d
@@ -141,9 +142,10 @@ final class DebugAllocator : Allocator {
this(Allocator base_allocator)
{
m_baseAlloc = base_allocator;
m_blocks = new HashMap!(void*, size_t)(manualAllocator());
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for the new? Couldn't we just use a plain HashMap here instead of a pointer?

s-ludwig
s-ludwig reviewed Oct 14, 2014
View reviewed changes
source/vibe/utils/memory.d
@@ -648,7 +650,7 @@ struct FreeListRef(T, bool INIT = true)
//logInfo("ref %s destroyed", T.stringof);
}
static if( hasIndirections!T ) GC.removeRange(cast(void*)m_object);
manualAllocator().free((cast(void*)m_object)[0 .. ElemSize+int.sizeof]);
static if (!INIT) manualAllocator().free((cast(void*)m_object)[0 .. ElemSize+int.sizeof]);
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add this static if, did this fix some kind of crash? If I'm not completely mistaken, this will create a memory leak for objects with INIT == true, destroy itself doesn't do any deallocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I saw a crash where there was an invalid memory operation because the .destroy seemed to have already gotten rid of the object. However, I can't remember how or when it had occurred, should be in the HTTPServer if I'm not mistaken. It was also back when I was testing LDC..

It looks like it's around the same time: ldc-developers/ldc#729

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is probably part of an interface issue in LDC. I'll remove it

Use HashMap in memory debugger
effc3d4 
Remove bad formatting
@s-ludwig
Copy link
Member

Okay thanks. I'll merge now.

invalid memory operation

Do you mean InvalidMemoryOperation or an access violation/segfault in this case?

s-ludwig added a commit that referenced this pull request Oct 14, 2014
@s-ludwig
Merge pull request #848 from etcimon/debug-alloc-invalid
e00f083 
Fix InvalidMemoryOperationError in DebugAllocator
@s-ludwig s-ludwig merged commit e00f083 into vibe-d:master Oct 14, 2014
@etcimon
Copy link
Contributor Author

etcimon commented Oct 14, 2014

I had an InvalidMemoryOperation infinite loop but I'm not sure if it's what this fix was for, because I also got segfault issues when testing LDC. That compiler has issues with calling the abstract version of a virtual function because it doesn't upcast properly according to my latest tests with native events. I had to disable invariants and tons of stuff to get to the root cause of it, and then I didn't investigate further.

@etcimon etcimon deleted the debug-alloc-invalid branch October 14, 2014 16:43
@s-ludwig
Copy link
Member

Hm, maybe it was just the initial workaround for the memory operation error in DebugAllocator.free? But I guess it will just show up again anyway if it's still there.

@etcimon
Copy link
Contributor Author

etcimon commented Oct 14, 2014

Hm, maybe it was just the initial workaround for the memory operation error in DebugAllocator.free? But I guess it will just show up again anyway if it's still there.

Could be. The interface problems are still there without the debug allocator though, I got ldc and llvm loaded into clion in an attempt to look for a clue about it. Is vibe.d supposed to be compatible with ldc?

@s-ludwig
Copy link
Member

Some reports had been positive, but I didn't yet successfully test it on LDC. In fact it crashes with a stack trace when I try to compile the http_server example. And earlier versions crashed at runtime within the fiber code in Druntime. But in principle I'd really like to get building with LDC to work.

@etcimon
Copy link
Contributor Author

etcimon commented Oct 14, 2014

But in principle I'd really like to get building with LDC to work.

It's going to take a few design changes in LDC I think, I've been reading the code for clang to get familiar with the LLVM way and I expect to get involved with that in 3-4 months. The LDC compiler was one of the reasons I started coding with D, it's the only way D comes on top of C and C++ while offering the pleasant high-level language facilities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
needs input
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@etcimon @s-ludwig