Sigsegv as exception #187

Merged
merged 14 commits into from Nov 29, 2012

10 participants

@deadalnix

When core.nullpointererror is imported in a project, it transform null deference into NullPointerError and other segfault in SignalError on linux x86 and x86_64 .

If the idea is successful, and implemented on other systems (windows, macOS, freeBSD) it could become the standard behavior. For now, it only behave that way when the module is explicitly imported.

This is realted to : #181 that should be included before.

@MartinNowak
D Programming Language member
  • Translating signals to exceptions is highly questionable.
    It was already a bad decision on windows and we shouldn't
    try to emulate it.

  • I'm rather worried about the ABI instabilities of ucontext_t.

  • Signal handler's are set per process.

    • you need to do something with existing handlers
    • what if a segfault occurs in a non-D thread
@MartinNowak MartinNowak and 1 other commented on an outdated diff Mar 28, 2012
src/core/nullpointererror.d
+ REG_EDX,
+ REG_ECX,
+ REG_EAX,
+ REG_TRAPNO,
+ REG_ERR,
+ REG_EIP,
+ REG_CS,
+ REG_EFL,
+ REG_UESP,
+ REG_SS
+ }
+}
+
+// Init
+
+shared static this() {
@MartinNowak
D Programming Language member
MartinNowak added a line comment Mar 28, 2012

This is odd.
It means that this mechanism is activated by importing the module.
Instead it should be a function like installHandler.

@deadalnix
deadalnix added a line comment Mar 28, 2012

That is fine to me. It can definitively be changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@deadalnix

Translating signals to exceptions is highly questionable. It was already a bad decision on windows and we shouldn't try to emulate it.

I think your point here is very weak. No argument whatsoever here. Let me state why I think it is a good idea.

  • it allow to recover from a null pointer deference. This isn't an error that screw up the state of your program and it is definitively recoverable. Throw allow to recover.
  • it provide a stacktrace for when things goes wrong. Even if you are not running tje program using a debugger.
  • you can exit your program nicely on null deference.
  • If you call something that is @safe, unless things goes REALLY wrong, in way that your program can do little about, you can now ensure that things will not crash. (deferencing null is @safe ).

Additionally, it opens some doors :

  • it is now possible to implement thing in a consistent manner on windows and linux, making easier to write cross plateform code.
  • the proposed code allow to do something else than throw. This can evolve in custom handler depending on the memory block (something similar to libsigsegv).
  • This type of tricks are mandatory to implement concurrent GC. It is a direction we want to go in.

Information have been asked to linux hackers, ucontext_t is supported, even if not very documented. You can ask feep if you doubt my claim.

About existing handler, I think an user playing with signal handler is big enough to know what he/she is doing and be sure to not collide with this. If the segfault occur in non-D thread (or even in non D code) things will work as well if you recover. If you throw this will go wrong, as D's Exception are not compatible with C/C++. Anyway, without the Exception, things goes wrong as well. A global thread local flag can be set to ensure this behavior doesn't trigger in non D threads.

@MartinNowak
D Programming Language member

it allow to recover from a null pointer deference.

No you cannot because you corrupt your complete program state at the point
you do not return from the signal. You might only call async safe functions after
that.

http://www.google.com/search?q=bug%20signal%20handler%20deadlock

provide a stacktrace for when things go

If you want to improve the current behavior then try to print
useful information from within the signal handler.
That will be hard but it might be possible, among other functions
you cannot use malloc/printf.

@deadalnix

I think you totally misunderstood the piece of code you are commenting. The whole point of the code in the handler is to provide a way for another piece of code OUTSIDE the handler, to handle the segfault.

The state of the program isn't corrupted, so it is safe to throw and even to recover from it. I even have a piece of code where I use mprotect to trigger segfault, and totally recover without throwing, using the exact same method.

It is recoverable, just try it if you don't believe me.

@MartinNowak
D Programming Language member

Yeah, you only reset the instruction pointer but that isn't much different
from performing a longjmp from inside the signal handler.

You still interrupt functions at arbitrary points, and exception handling
won't help you with scope cleanup because the code is not prepared
to handle them. These are similar issues as with exception-unsafe code,
only that the compiler will make it even worse and it may occur at
any instruction.

Logger logger;
void setLogger()
{
    logger = new Logger;
    scope (failure) logger = null;
    logger.init(getSomething());
}

If getSomething caused a segfault your program is corrupted.
Nobody will clean up the initializer, because the compiler thinks
getSomething is nothrow.

Likewise if a segfault happened in malloc you may not call it again.
It isn't async-safe and you have left it in whatever state it was.

@deadalnix

No, this isn't like longjmp ! In that case, you know that the whole program is back in the right state (longjmp doesn't guarantee that).

And the only registers altered are trash registers, so it is safe in regard of try catch blocks. For you example, it currently works (tested as well) with the code generated by dmd.

I feel like you are making up problems that doesn't even exists here, and you don't even consider the advantages that such a mechanism provide.

@JakobOvrum
D Programming Language member

If the idea is successful, and implemented on other systems (windows, macOS, freeBSD) it could become the standard behavior.

This is already implemented on Windows as the default behaviour. This needs more thought; with this patch it will even throw different exceptions on Linux and Windows, which is a completely unnecessary inconsistency.

@deadalnix

Yes, this is inconsistent. And yes, this needs to be made consistent with windows.

The point is that the exception thrown on windows is system specific. I would rather think that both should throw a NullPointerError on null deference, so we have a system agnostic way of catching this. This definitively need to converge.

I didn't wanted to change existing behavior on windows, so this approach can be tested without breaking anything. Ultimately, this is linked to the problem of @safe that allow unsafe things to be done when passed large objects/pointers that are null. What must be done to solve that problem is something we have to decide before changing the existing behavior, or we risk to change it twice, which is something we want to avoid.

@FeepingCreature

For reference, I cite ##kernel on freenode: http://pastebin.com/VEeZYPRJ . So it seems to be "officially" supported.

The main advantage over longjmp is that it definitely ends up with the handler function called from a state that is "safe", since it reuses the kernel's existing return-to-previous-context mechanism. So you don't have to emulate whatever cleanup the kernel does at signal handler exit.

@andralex
D Programming Language member

Undecided on what to do about this. Should we discuss the matter further in the newsgroup? I know @WalterBright has a dim view on converting null pointer accesses into exceptions.

@andralex
D Programming Language member

(it ain't rebased either :o))

@deadalnix

Indeed, this pull request involve a language design decision, and may be discussed in the newsgroup. The problem to be fixed is in fact much larger than what this pull request does, it is about the whole null deference handling in D.

This pull request open the door for unified behavior between linux and windows. It is also easy to provide a callback to do whatever is wanted (HALT if one think is better, or throw). It is even possible to recover in some situation (concurrent GC is a great use case of that capability for instance).

Note that I use this in all code I produce in D until then, and it have been of great help.

@deadalnix

I rebased the pull request. git wasn't able to merge the stuff by itself.

@MartinNowak
D Programming Language member

There are still tons of issues.

Your signal handler is global.

  • Do you throw D exceptions in any language?
  • What to do when you overwrite an existing signal?

This applies to all D executables and all D libraries.

  • Would you want libfontconfig to steal your sigsegv handler?

You'll get deadlocks which are even worse than crashing.

  • What to do w.r.t. signal-unsafe functions?

You cannot recover without unwinding/destruction support.

  • How could this be implemented with enregistered variables?

  • Why shall we make a non-standard unsafe function which will cause difficult to find
    bugs the default behavior when you can make it a library?

This has absolutely no place in production code because it is unpredictable and unreliable.

@deadalnix

I'm sorry, but I don't think most of this are problems.

First, yes, it throw D exception, but not in any language, because IT IS IN D RUNTIME. So it throw D exception in D. If you interface D with other languages, then you have to make sure that the interfacing make sense. This is no news and have nothing to do with that pull request specifically.

As of 3rd party lib stealing the signal handler, it is a possibility. However, the signal mechanism is already used in druntime. So either we consider this is a problem, or we don't. But this is rather stupid to state this is a problem when this is done all over the place.

Many other remarks show that you don't understand what is going on here. The whole mechanism is here to set up a function call on top of the instruction that caused the fault. So everything happen in userland, not in the signal handler. Signal usafe function will work properly, and no deadlock will occur.

I'm sorry, but I see nothing but FUD in your comment. You should come with actual fact here.

@FeepingCreature

I have to agree with this. See my earlier paste from ##kernel - it's standard, albeit uncommon. The technique works on any operating system that uses the basic x86 stackframe layout, which, I think, is more systems than D runs on - and it's not like "but this only works on Linux" stopped people when it came to supporting SEH. And the entire point of this is to sidestep the issue of signal safety. Let's not get into flames - but please make sure you have read the entire thing before objecting.

@deadalnix

@dawgfoto please excuse me for the rudeness of my previous message. I let it here to keep the discussion understandable, but It was way too aggressive. I'm sorry.

To restate thing in a more neutral way : I have nothing against this not being included, but it have to be discussed and the decision must be taken based on actual hard facts. Please feel free to ask any question about the technical aspect of things, because your post contained inaccurate informations. As I'm pretty sure it was not done on purpose, I guess we simply have a misunderstanding. So let's start again on good basis, and please forgive my tone.

@andralex
D Programming Language member

@deadalnix Thanks very much. All - let's keep the good spirits going! I'll ask Walter what he thinks about the idea. Is this implementable on all major OSs?

@deadalnix

On windows, the system already throw an Exception when this happen. A similar mechanism can be implement on windows without much problems.

I'm not qualified enough on FreeBSD or macOS to answer that question.

@MartinNowak
D Programming Language member

Many other remarks show that you don't understand what is going on here. The whole mechanism is here to set up a function call on top of the instruction that caused the fault. So everything happen in userland, not in the signal handler. Signal usafe function will work properly, and no deadlock will occur.

void* p = void; // uninitialized
core.stdc.free(p);
// malloc.c
void free(void* p)
{
    // ...
    rlock_acquire(&malloc_mtx);
    size_t len = *cast(size_t)(p-1); // SIGSEGV
}

Now your preemptively exited a signal-unsafe function and you have no way of repairing it.
From now on every call to malloc/realloc/calloc/free might dead lock.

A set of functions that you may call is listed in signal(7) - Async-signal-safe functions.

it allow to recover from a null pointer deference. This isn't an error that screw up the state of your program and it is definitively recoverable. Throw allow to recover.

If your talking about recovering you need a mechanism to unwind the stack and restore state.
The one used for synchronous exceptions doesn't scale to asynchronous exceptions
because the compiler has to dump all variables to the stack in order to access them from exception handlers.

See my earlier paste from ##kernel - it's standard, albeit uncommon.
Altering the instruction pointer and continuing execution somewhere else is not the issue.

@MartinNowak
D Programming Language member

Is this implementable on all major OSs?

It's common that sigreturn restores the CPU context from the signal's ucontext_t as far as security allows.
FreeBSD - sys_sigreturn
OSX

@deadalnix

I understand your example with malloc/free. You have to understand that in this case, whatever happen, you are doomed. Either the program crash either it is in an inconsistent state. It is a situation where catching the NullPointerError make no sense t all, because you can't recover from it.

Not having this behavior will not solve the problem, because you'll also be in an inconsistent state or you'll crash. This isn't any better and I don't see how this pull request is making things worse. A invalid call to a system function have been made, whatever comes out from that is either a crash or a inconsistent state.

Note that those function are signal unsafe. And this pull request don't change in any way if signal are send or received, it just change how they are handled. At the moment the code present in this pull request start to execute, the arm is already done, and the program is already in a beyond repair state. It is in that state because of the signal, not because of the code present in the pull request, so I don't see how it is an argument for or against it.

Another fact here is that the exception is thrown from C code and C don't handle exception. This problem isn't specific to this pull request, this is a problem that can occur every time an exception is thrown throw C code. And this problem is unsolvable, as C don't support exceptions. It is up to the programmer to ensure that exception are not throw throw C code.

This is exactly why I inherited NullPointerError from Error and not from Exception. They are not always recoverable. But they always are in @safe code.

@MartinNowak
D Programming Language member

This isn't any better

On an automated system deadlocks are worse than failures.
So we'd need either an opt-in or an opt-out switch.

By the way could you rephrase the purpose of translating signals to errors?
If it's error reporting you could do way more advanced stuff using dumps, execve, fork and ptrace.
For example you could restart the process in an error-reporting mode. You could also fork it first and
enable ptrace so that you may inspect memory from the reporter.

immutable pid = fork();
if (pid)
{
    char[11] buf=void;
    format(buf, pid);

    const char* args[3];
    args[0] = argv[0]; // C argv (this has issues with chdir, deleted images, changed rights...)
    args[1] = "--druntime-report-error";
    args[2] = buf.ptr;
    execve(args[0], args.ptr, environ);
}
else
{
    ptrace(PT_TRACE_ME);
    abort();
}
@FeepingCreature

Personally: exceptions/errors have backtraces. Backtraces are immensely useful (no, gdb is not the answer). It'd also add consistency with Windows.

Look. Of course we can get backtraces for segfaults under linux with effort. But this patch allows us to get backtraces without effort, and that level of trivial convenience has a quality of its own.

About your free example: worst case, you can always inspect the stack, see if you're called from free(), and manually unlock. In point of fact, I think C free under Linux can tell that its argument is not a valid pointer and give a proper error [edit my mistake: it does segfault]. That aside, without this patch it just dies. With this patch it maybe dies if the exception is uncaught. All it does is add the option to do proper cleanup. And generally speaking, if you're catching an Error you deserve what you get. It's a big red flag that says "This guy think he know what he doin".

@MartinNowak
D Programming Language member

The point is that creating a backtrace might be enough to do way more harm.
Reliable crash information can be generated from another process (FF, Chrome, Ubuntu, OSX, Windows...).
If we come up with a solid solution it might be useful for deployed applications too.

How about a library solution, for example?
http://code.google.com/p/google-breakpad/

@deadalnix

I also think that should evolve to provide a custom handler for advanced user.

As of now, it is in opt-in, you have to include this module somewhere to « activate » it.

@MartinNowak
D Programming Language member

Why doesn't this simply restore BP and call _d_throwc from within the signal handler?
That would also allow us to use sigaltstack for handling stack overflows.

@deadalnix

Throwing from the signal handler isn't safe. You aren't in a standard execution flow, and the kernel knows it. Yes the code can be extended to manage stack overflows, and should be IMO.

@MartinNowak
D Programming Language member

Throwing from the signal handler isn't safe.

For the same reason that this mechanism is unsafe or am I missing something.

You are in a standard execution flow, and the kernel knows it.

What do you mean by that?

@deadalnix

Throw from the signal handler is always unsafe, because you aren't in a regular execution flow.

This mecanism is unsafe in situation that are unsafe anyway, with or without. For instance, calling free with an invalid parameter will cause this mecanism to be unsafe. But it is unsafe anyway.

In @safe code, this mecanism is 100% safe. Throwing from the signal handler isn't in such a situation.

@MartinNowak
D Programming Language member

Throw from the signal handler is always unsafe, because you aren't in a regular execution flow.

Do you mean the signal stack frame?
We don't need to execute on the original stack to unwind it.

This mecanism is unsafe in situation that are unsafe anyway, with or without. For instance, calling free with an invalid parameter will cause this mecanism to be unsafe. But it is unsafe anyway.

One wouldn't trap a segmentation fault if the executed code was alright.

In @safe code, this mecanism is 100% safe. Throwing from the signal handler isn't in such a situation.

A segfault in @safe code implies that you invoked that code with invalid arguments or an invalid program state. The reason executing any further code, within or after the signal handler, is unsafe remains the same.
Because you exited code with an asynchronous exception any invariant might be broken, malloc being a simple example.
Because the fault might be a result of an earlier error one cannot assume localized corruption.

http://www.gnu.org/software/libc/manual/html_node/Nonreentrancy.html#Nonreentrancy
http://www.gnu.org/software/libc/manual/html_node/Defining-Handlers.html#Defining-Handlers
http://pubs.opengroup.org/onlinepubs/009695399/xrat/xsh_chap02.html#tag_03_02_04_04
http://austingroupbugs.net/view.php?id=516

@FeepingCreature

Do you mean the signal stack frame?
We don't need to execute on the original stack to unwind it.

To my knowledge, it may be possible to unwind from a signal handler, but there's nothing in the spec that guarantees it's safe (or that the stack even exists between signal handler and rest of function). In fact, the only guaranteed safe way to exit a signal handler is via return. Thus this hack.

@klickverbot klickverbot commented on an outdated diff Jul 18, 2012
src/core/nullpointererror.d
@@ -0,0 +1,283 @@
+/**
+ * Written in the D programming language.
@klickverbot
D Programming Language member
klickverbot added a line comment Jul 18, 2012

Should not be part of the doc comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@klickverbot klickverbot and 1 other commented on an outdated diff Jul 18, 2012
src/core/nullpointererror.d
@@ -0,0 +1,283 @@
+/**
+ * Written in the D programming language.
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
@klickverbot
D Programming Language member
klickverbot added a line comment Jul 18, 2012

Spelling (dependant).

@alexrp
D Programming Language member
alexrp added a line comment Jul 18, 2012

(and dereferencing, not deferencing)

I'd reword to:

Handle page protection errors using Errors. NullPointerError is thrown when dereferencing null. A system-dependent error is thrown in other cases.

Also see my comments about NullPointerError.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@klickverbot klickverbot and 1 other commented on an outdated diff Jul 18, 2012
src/core/nullpointererror.d
@@ -0,0 +1,283 @@
+/**
+ * Written in the D programming language.
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
+ * Note : Only linux on x86 and x86_64 is supported for now.
@klickverbot
D Programming Language member
klickverbot added a line comment Jul 18, 2012

Remove the space in front of :, capitalize Linux.

@alexrp
D Programming Language member
alexrp added a line comment Jul 18, 2012

and s/is/are/.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@klickverbot klickverbot commented on an outdated diff Jul 18, 2012
src/core/nullpointererror.d
@@ -0,0 +1,283 @@
+/**
+ * Written in the D programming language.
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
+ * Note : Only linux on x86 and x86_64 is supported for now.
+ *
+ * Copyright: Copyright Digital Mars 2000 - 2012.
@klickverbot
D Programming Language member
klickverbot added a line comment Jul 18, 2012

If you wrote the code, it's most probably not »Copyright Digital Mars 2000 - 2012.«.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@klickverbot klickverbot and 2 others commented on an outdated diff Jul 18, 2012
src/core/nullpointererror.d
+ static REG_TYPE saved_RDI, saved_RSI;
+
+ extern(C)
+ void handleSignal(int signum, siginfo_t* info, void* contextPtr) {
+ auto context = cast(ucontext_t*)contextPtr;
+
+ // Save registers into global thread local, to allow recovery.
+ saved_RDI = context.uc_mcontext.gregs[REG_RDI];
+ saved_RSI = context.uc_mcontext.gregs[REG_RSI];
+
+ // Hijack current context so we call our handler.
+ auto rip = context.uc_mcontext.gregs[REG_RIP];
+ auto addr = cast(REG_TYPE) info.si_addr;
+ context.uc_mcontext.gregs[REG_RDI] = addr;
+ context.uc_mcontext.gregs[REG_RSI] = rip;
+ context.uc_mcontext.gregs[REG_RIP] = (rip != addr)?(cast(REG_TYPE) &sigsegv_userspace_handler + 0x04):(cast(REG_TYPE) &sigsegv_userspace_handler);
@klickverbot
D Programming Language member
klickverbot added a line comment Jul 18, 2012

This can be shortened, no? Also, the line is somewhat long.

@deadalnix
deadalnix added a line comment Jul 18, 2012

It cannot be that much shortened. It is necessary to handle the specific case where the memory access is done to execute the code. IE :

function() foo = null;
foo();

@MartinNowak
D Programming Language member
MartinNowak added a line comment Jul 18, 2012

Please use two different functions instead of jumping to an offset.

@deadalnix
deadalnix added a line comment Jul 18, 2012

I understand your concern, but this lead to massive code duplication, I'm not sure this is really better. What do other people think ?

@klickverbot
D Programming Language member
klickverbot added a line comment Jul 18, 2012

@deadalnix: cast(REG_TYPE) &sigsegv_userspace_handler + ((rip != addr) ? 0x4 : 0)? But I really just wanted to mention that we generally try to keep lines below ~100 characters in druntime/Phobos.

@klickverbot
D Programming Language member
klickverbot added a line comment Jul 18, 2012

Oh, and if you stick with this, you should definitely document the fact that you rely on the offset in the implementation below, so that nobody accidentally breaks the code in the future.

@deadalnix
deadalnix added a line comment Jul 18, 2012

OK, I'll rewrite thing to get shorter lines/

I'll add a comment here about the offset trick, one is already present in the function the code jump to. I agree this is not trivial and is important to mention.

@MartinNowak
D Programming Language member
MartinNowak added a line comment Jul 18, 2012

but this lead to massive code duplication

No duplication needed.
Just create two naked asm thunks that pass over to sigsegv_userspace_handler.
This is also the right place to add the missing stack alignment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp alexrp commented on an outdated diff Jul 18, 2012
src/core/nullpointererror.d
+ pop EBP;
+ ret;
+ }
+ }
+
+ // The return value is stored in EAX and EDX, so this function restore the correct value for theses registers.
+ REG_TYPE[2] restore_registers() {
+ return [saved_EAX, saved_EDX];
+ }
+}
+
+// This should be calculated by druntime.
+enum PAGE_SIZE = 4096;
+
+// The first 64Kb are reserved for detecting null pointer deferences.
+enum MEMORY_RESERVED_FOR_NULL_DEFERENCE = 4096 * 16;
@alexrp
D Programming Language member
alexrp added a line comment Jul 18, 2012

s/DEFERENCE/DEREFERENCE/g

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@klickverbot klickverbot commented on an outdated diff Jul 18, 2012
src/core/nullpointererror.d
@@ -0,0 +1,283 @@
+/**
+ * Written in the D programming language.
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
+ * Note : Only linux on x86 and x86_64 is supported for now.
+ *
+ * Copyright: Copyright Digital Mars 2000 - 2012.
+ * License: Distributed under the
+ * $(LINK2 http://www.boost.org/LICENSE_1_0.txt, Boost Software License 1.0).
+ * (See accompanying file LICENSE_1_0.txt)
+ * Authors: Amaury SECHET, FeepingCreature, Vladimir Panteleev
+ * Source: $(DRUNTIMESRC src/core/nullpointererror.d)
+ */
+module core.nullpointererror;
+
+version(linux) {
@klickverbot
D Programming Language member
klickverbot added a line comment Jul 18, 2012

Generally, new code in druntime and Phobos should follow the style conventions, i.e. braces on a separate line, and 4 space indentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp alexrp and 2 others commented on an outdated diff Jul 18, 2012
src/core/nullpointererror.d
+ return [saved_EAX, saved_EDX];
+ }
+}
+
+// This should be calculated by druntime.
+enum PAGE_SIZE = 4096;
+
+// The first 64Kb are reserved for detecting null pointer deferences.
+enum MEMORY_RESERVED_FOR_NULL_DEFERENCE = 4096 * 16;
+
+// User space handler
+
+void sigsegv_userspace_process(void* address) {
+ // The first page is protected to detect null deference.
+ if((cast(size_t) address) < MEMORY_RESERVED_FOR_NULL_DEFERENCE) {
+ throw new NullPointerError();
@alexrp
D Programming Language member
alexrp added a line comment Jul 18, 2012

Not NullPointerError. A seg fault isn't necessarily a null dereference. Please call it MemoryError or something like that.

@deadalnix
deadalnix added a line comment Jul 18, 2012

That is why a check is performed. In the general case, a SignalError is thrown.

As mentioned below, this should definitively evolve into something that check for StackOverflow and allow 3rd party code to hook its logic on a precise memory page.

@alexrp
D Programming Language member
alexrp added a line comment Jul 18, 2012

I understand, but dereferencing a value like 0x10 would yield a NullPointerError too, which would be very confusing. Perhaps InvalidPointerError would be more accurate.

@deadalnix
deadalnix added a line comment Jul 18, 2012

class A {
int a;
int b;
}

A a = null;
a.b; // null dereference with an address > 0 .

More generally, Linux define the file /proc/sys/vm/mmap_min_addr that define how much memory is reserved by the system to handle null pointer dereferences. It seems to me that this is reasonable to use the same value here.

@alexrp
D Programming Language member
alexrp added a line comment Jul 18, 2012

I'm not sure I follow. The way I understand it,

auto foo = cast(ubyte*)0x10;
*foo = 1;

would result in a NullPointerError with your current code, no?

@FeepingCreature
FeepingCreature added a line comment Jul 18, 2012

This should maybe be SegmentationFaultError. For instance, another way to get this would be when accessing freed memory.

@deadalnix
deadalnix added a line comment Jul 18, 2012

@alexrp yes, this will be detected as null dereference. Such a code is unsafe and such a pointer can only be forged the way you did it. Except for null, no function will ever give you such a pointer.

@FeepingCreature no. No memory can be mapped at this address. Never ever. The only way you can get a pointer to such an address it either to have a null pointer or to set it manually like @alexrp does.

@FeepingCreature
FeepingCreature added a line comment Jul 18, 2012

@deadalnix My apologies. You are correct.

@alexrp
D Programming Language member
alexrp added a line comment Jul 18, 2012

I think you're a little too focused on @safe code. Keep in mind that this is a systems language; SafeD is just a subset.

Consider:

ubyte* ptr = func(); // returns null due to some silly bug
ubyte val = *(ptr + 0x10);

This is effectively the same as my first example, but shows a case where it's more likely to happen. You don't have to forge an invalid address intentionally for this to happen.

I'm willing to compromise: Make an InvalidPointerError class and a NullPointerError class (which derives from InvalidPointerError). Throw the latter when you know that the pointer was null (and nothing else); otherwise, the former.

@deadalnix
deadalnix added a line comment Jul 18, 2012

Expect for names, this is what is done. NullPointerError inherit from SignalError . InvalidPointerError is a better name, I'll rename that, but the mechanism is here.

@alexrp
D Programming Language member
alexrp added a line comment Jul 18, 2012

The way I read it, anything lower than MEMORY_RESERVED_FOR_NULL_DEFERENCE will yield a NullPointerError...? That is to say, my above example will throw the misleading NullPointerError. That's what I don't want. When I dereference a value like 0x10, I want an InvalidPointerError; otherwise (when the pointer was definitely null and nothing else), a NullPointerError.

@deadalnix
deadalnix added a line comment Jul 18, 2012

I see, I misunderstood your point above.

A NullPointerError IS a SignalError (that will be renamed into InvalidPointerError as you suggested).

Your example above is, to me, suitable for a NullPointerError, because func did returned null and you dereference based on that. See the code as :
ubyte* ptr = func(); // returns null
ubyte val = ptr[0x10];

This type of stuff will generate a NullPointerSomething in almost every language, even if the address accessed isn't specifically null. This is why /proc/sys/vm/mmap_min_addr is often 64k instead of 4k.

Accessing this memory can only be done by forging a pointer manually or basing a dereference on null (as you did in the example above).

@alexrp
D Programming Language member
alexrp added a line comment Jul 18, 2012

Well, the only languages I know that throw NPE/NRE on any invalid memory access are Java and C# (.NET in general). Java does it because Java doesn't have pointers in the first place, so anything else wouldn't make sense. C# does it due to Java heritage and because pointers were an afterthought in the design. In fact, I've had plenty of "wat" moments in C# when my invalid pointer accesses resulted in a NullReferenceException when nothing was null. I find it very unintuitive, personally, because when I write pointer operations I don't think about what they may be at a higher level; I think about what they actually are on machine level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp alexrp commented on an outdated diff Jul 18, 2012
src/core/nullpointererror.d
+ // The return value is stored in EAX and EDX, so this function restore the correct value for theses registers.
+ REG_TYPE[2] restore_registers() {
+ return [saved_EAX, saved_EDX];
+ }
+}
+
+// This should be calculated by druntime.
+enum PAGE_SIZE = 4096;
+
+// The first 64Kb are reserved for detecting null pointer deferences.
+enum MEMORY_RESERVED_FOR_NULL_DEFERENCE = 4096 * 16;
+
+// User space handler
+
+void sigsegv_userspace_process(void* address) {
+ // The first page is protected to detect null deference.
@alexrp
D Programming Language member
alexrp added a line comment Jul 18, 2012

dereference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp alexrp commented on an outdated diff Jul 18, 2012
src/core/nullpointererror.d
+ this(int signum, Throwable next, string file = __FILE__, size_t line = __LINE__) {
+ _signum = signum;
+ super("", file, line, next);
+ }
+
+ /**
+ * Property that returns the signal number.
+ */
+ @property
+ int signum() const {
+ return _signum;
+ }
+}
+
+/**
+ * Throw on null pointer deference.
@alexrp
D Programming Language member
alexrp added a line comment Jul 18, 2012

dereference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp alexrp and 1 other commented on an outdated diff Jul 18, 2012
src/core/nullpointererror.d
@@ -0,0 +1,283 @@
+/**
+ * Written in the D programming language.
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
+ * Note : Only linux on x86 and x86_64 is supported for now.
+ *
+ * Copyright: Copyright Digital Mars 2000 - 2012.
+ * License: Distributed under the
+ * $(LINK2 http://www.boost.org/LICENSE_1_0.txt, Boost Software License 1.0).
+ * (See accompanying file LICENSE_1_0.txt)
+ * Authors: Amaury SECHET, FeepingCreature, Vladimir Panteleev
+ * Source: $(DRUNTIMESRC src/core/nullpointererror.d)
+ */
+module core.nullpointererror;
@alexrp
D Programming Language member
alexrp added a line comment Jul 18, 2012

I don't quite like this module name (see my comment about NullPointerError below). Some ideas: core.error, core.segfault, ...

@deadalnix
deadalnix added a line comment Jul 18, 2012

Agreed. I'll rename that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MartinNowak
D Programming Language member

To my knowledge, it may be possible to unwind from a signal handler

It's the same as doing it after sigreturn.

but there's nothing in the spec that guarantees it's safe

Because it isn't, but I don't see a reason why it would be safer after sigreturn, do you?

(or that the stack even exists between signal handler and rest of function).

Trapping a signal doesn't alter any memory mapping.

In fact, the only guaranteed safe way to exit a signal handler is via return. Thus this hack.

You can abort(3) the process, reraise(3) the signal or execve(2) a different image.
Those are safe operations, but sigreturn(2)ing from the handler, longjmp(3)ing to a restore point or unwinding from the handler are not because the error signal indicates a corrupted process. A null pointer dereference isn't any different because one doesn't know if it's a consequence of an earlier corruption.

The open questions are how much to risk for extended error information, how to make it as portable as possible and how to make it configurable.
As I've pointed to earlier it's also feasible to do this in a safe manner.

@FeepingCreature
In fact, the only guaranteed safe way to exit a signal handler is via return. Thus this hack.

You can abort(3) the process, reraise(3) the signal or execve(2) a different image.
Those are safe operations, but sigreturn(2)ing from the handler, longjmp(3)ing to a restore point or unwinding from the handler are not because the error signal indicates a corrupted process.

My apologies, I meant "safe way to exit a signal handler while preserving the stack", and I specifically meant "safer than just unwinding through the signal handler because you never know what the kernel might have done setting up the signal." I agree that it is generally unsafe to continue a program after potential corruption has occurred, but a) null pointer access as opposed to just any segment violation generally indicates an error that is probably not destructive since it seems unlikely that other memory would have been overwritten, and b) this still gives us the chance to do cleanup and print a stack trace, which is incredibly useful (and no, gdb is not a substitute). And hey, if you want to propose some better way of doing backtraces, as the open source people say - patches welcome.

I just think the ultimate decision how to handle errors should lie with the programmer, not the runtime.

@deadalnix

First of all, let's consider when this thing can be triggered. You'll find 2 cases :
1/ When a null dereference occurs. This can be anywhere, even in @safe code, this is common and non destructive.
2/ in @system code, when things get corrupted.

As far as I understand, nobody have any argument against situation 1.

Situation 2 is the case where the program cannot be recoverable, as @dawgfoto states. This is true, but in a first place, the program isn't screwed by this patch, it is screwed way before. In such a situation, how the fact the an Error percolate is worse than the program aborting abruptly ?

Situation 2 look like more an argument against @system code than against this proposal precisely. @system code can screw up the integrity of the program, this is know fact, and as soon as @system is around ANYTHING can happen.

@MartinNowak MartinNowak and 1 other commented on an outdated diff Jul 18, 2012
src/core/nullpointererror.d
+
+ pop RDI;
+
+ // Restore trash registers value.
+ pop R11;
+ pop R10;
+ pop R9;
+ pop R8;
+ pop RDX;
+ pop RCX;
+ pop RAX;
+ popf; // Restore flags.
+
+ // Return
+ pop RBP;
+ ret;
@MartinNowak
D Programming Language member
MartinNowak added a line comment Jul 18, 2012

Why are you saving/restoring the error state?
You cannot continue execution at the failing instruction, i.e. sigsegv_userspace_process must not return.
That also implies you can scratch the code vs. data distinction too.

@deadalnix
deadalnix added a line comment Jul 24, 2012

If the segfault is triggered by an mprotect you can get back in the program just like if nothing happened. The goal is to allow any code to be plugged here, and not just throw exception, and full recovery is actually possible in many cases (and yes, this is tested).

@MartinNowak
D Programming Language member
MartinNowak added a line comment Jul 25, 2012

If the segfault is triggered by an mprotect you can get back in the program just like if nothing happened.

Right, but what's the use case for accessing protected pages?
Are you thinking of GC read/write-barriers?

If you want to support arbitrary code you need to go the full way, e.g. saving and defaulting the FPU state.

@deadalnix
deadalnix added a line comment Jul 30, 2012

Isn't the FPU supposed to be saved by the callee ? If not, indeed, I have to add that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MartinNowak
D Programming Language member

safer than just unwinding through the signal handler

Let's end that branch of the discussion. I just proposed that unwinding in the signal handler would allow us to use sigaltstack(2) and extend this to stack overflows.

I just think the ultimate decision how to handle errors should lie with the programmer, not the runtime.

Right, but this pull is about what error handling schemes the runtime offers.
Though I do think this one would be useful for certain development workflows.

Considering null pointer dereferencing.

@safe void foo(Bar bar)
{
    bar.func1();
    bar.func2();
}

If foo is called with a corrupted bar instance then calling func1 might do arbitrary damage before calling func2 might trap a null pointer access.

Has anyone considered to leverage the unittestSegvHandler. This wouldn't be nearly as controversial and could be done with two simple core.runtime functions enable/disableBacktraceOnSignals.

@andralex
D Programming Language member

Since discussion is in flux, I'm deferring approval to a mini-committee of the union of all reviewers of this. Generally I'm favorable to merging this in if it just works and is robust.

@WalterBright
D Programming Language member

As Andrei mentioned, I don't think this is a good idea. While I did it for Windows long ago, it hasn't panned out to be a useful or desirable feature. It's also a hard thing to get right, and requiring D implementations to do this could put a heavy burden on an implementation and a port (very few programmers are knowledgable enough to write such code).

@alexrp
D Programming Language member

@WalterBright Given that this module only activates itself when imported, it doesn't place any particular requirement on D implementations. Implementations that don't yet support it can simply do a static assert(false);.

I think the idea behind the module is fine since it is opt-in and not forced (like access violation errors on Windows...). But I still object to throwing NullPointerError for memory accesses that aren't actually accessing the value 0x0.

@MartinNowak
D Programming Language member
  • There are still many implementation issues not yet addressed.
  • We already have a more robust solution for backtraces on signals unittestSegvHandler.
  • I don't like the idea to allocate and throw an Error object, risking deadlocks, only to advise that running any catch code is unsafe.
@deadalnix

As @dawgfoto mentioned, some changes have to be made (see comments on code itself).

The Error object allocation is an issue that can be solved by preallocating the Error, or halting depending on the stack trace (I prefers the second one).

@WalterBright I don't think you can judge the usefulness of that only based on the windows version of it. First, the behavior isn't consistent across platforms, so it is avoided, and it don't separate what is a null dereference and a memory corruption. It a mechanism that is present in already successful languages and has proven itself useful and successful.

What do you think make that feature useful in java or C#, but not in D ?

@andralex Where do you want the discussion to take place ?

@CyberShadow
D Programming Language member

While I did it for Windows long ago, it hasn't panned out to be a useful or desirable feature.

Why? I do not recall any arguments against the feature, but I know many favor it. Re-running the program under a debugger on Windows is a good deal more cumbersome than with gdb.

I don't like the idea to allocate and throw an Error object, risking deadlocks

OutOfMemoryError and InvalidMemoryOperationError both are thrown using their .init, avoiding any kind of memory allocations.

@JakobOvrum
D Programming Language member

OutOfMemoryError and InvalidMemoryOperationError both are thrown using their .init, avoiding any kind of memory allocations.

Being really pedantic here, but the .init of a class type is just a null reference. I think you meant to say they are pre-allocated.

@CyberShadow
D Programming Language member
@JakobOvrum
D Programming Language member

Yes, I meant classinfo.init. See here: https://github.com/D-Programming-Language/druntime/blob/master/src/core/exception.d#L502

Wow, that's a really neat trick.

@MartinNowak
D Programming Language member

but I know many favor it. Re-running the program under a debugger

There is no need to conflate error diagnosis and exceptions.

both are thrown using their .init, avoiding any kind of memory allocations

That's avoids the allocation part but not the throw part.

@CyberShadow
D Programming Language member

I favor this pull request specifically for its aid in error diagnosis. Personally, I would be equally happy with D programs printing a stack trace and immediately exiting upon a SIGSEGV.

@andralex
D Programming Language member

I think it's difficult to put this in the language; as Walter said, this puts pressure on alternative implementations. But I think putting it in etc/linux would be a great way to test it out there and see how it works for people. What do you all think?

@CyberShadow
D Programming Language member

Just saw the edit:

That's avoids the allocation part but not the throw part.

What is the exact problem with throw? You can see the implementation here: https://github.com/D-Programming-Language/druntime/blob/master/src/rt/deh2.d#L183

@WalterBright
D Programming Language member

I haven't found any use for it under Windows, nor have I seen any.

I know that some programs rely on catching null pointer exceptions in Java, but I think that such is execrable program design. I also recall Andrei's arguments about using it to debug when using a debugger is impractical. Putting it in etc/linux as an unofficial/experimental module may be the best way.

@CyberShadow
D Programming Language member

I think the usefulness boils down to "When the program crashes, give me a sensible amount of information that can help me diagnose the problem right away, so I won't have to waste time setting up a debugger and trying to reproduce the crash there". As has been discussed previously, some classes of bugs in some classes of applications can take hours or more to reproduce. The same reasons why we print stack traces on unhandled exceptions generally, really.

@deadalnix

@WalterBright yes, relying on NPE in java isn't really a good idea. It isn't in D either.

However, as both Java and D have references types set as null by default, NPE WILL occur in any non trivial program. We should either prevent the proliferation of null (hello Rust - and to be fair, I think they are right, but it is another topic) or provide a way to handle it in the language.

Not providing any mecanism for that in C/C++ has proven to be a bad idea. It is the source of many security flaws, and some very bad ones (root exploit for instance often are created by unchecked null).

@andralex putting that in etc/linux and see how it goes is actually a really good idea.

@MartinNowak MartinNowak commented on an outdated diff Jul 25, 2012
src/core/nullpointererror.d
+
+ // This function must be called with faulting address in RDI and original RIP in RSI.
+ void sigsegv_userspace_handler() {
+ asm {
+ naked;
+
+ // Handle the stack for an invalid function call (segfault at RIP).
+ push RBP;
+ mov RBP, RSP;
+
+ // We jump directly here if we are in a valid function call case.
+ push RSI; // return address (original RIP).
+ push RBP; // old RBP
+ mov RBP, RSP;
+
+ pushf; // Save flags.
@MartinNowak
D Programming Language member
MartinNowak added a line comment Jul 25, 2012

pushfd/pushfq

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MartinNowak MartinNowak commented on an outdated diff Jul 25, 2012
src/core/nullpointererror.d
+ push RAX;
+
+ call restore_RSI;
+ mov RSI, RAX;
+
+ pop RDI;
+
+ // Restore trash registers value.
+ pop R11;
+ pop R10;
+ pop R9;
+ pop R8;
+ pop RDX;
+ pop RCX;
+ pop RAX;
+ popf; // Restore flags.
@MartinNowak
D Programming Language member
MartinNowak added a line comment Jul 25, 2012

popfd/popfq

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MartinNowak MartinNowak and 1 other commented on an outdated diff Jul 25, 2012
src/core/nullpointererror.d
+ // We jump directly here if we are in a valid function call case.
+ push RSI; // return address (original RIP).
+ push RBP; // old RBP
+ mov RBP, RSP;
+
+ pushf; // Save flags.
+ push RAX; // RAX, RCX, RDX, and R8 to R11 are trash registers and must be preserved as local variables.
+ push RCX;
+ push RDX;
+ push R8;
+ push R9;
+ push R10;
+ push R11;
+
+ // Parameter address is already set as RAX.
+ call sigsegv_userspace_process;
@MartinNowak
D Programming Language member
MartinNowak added a line comment Jul 25, 2012

The stack is probably not aligned at the point of this call.

@deadalnix
deadalnix added a line comment Jul 31, 2012

It for 64bits but isn't for 32, you are right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@deadalnix

I updated the pull request according to remarks of people here.

@JakobOvrum JakobOvrum commented on an outdated diff Jul 31, 2012
src/etc/linux/nullpointererror.d
@@ -0,0 +1,287 @@
+/**
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
@JakobOvrum
D Programming Language member
JakobOvrum added a line comment Jul 31, 2012

is thrown
dereferencing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JakobOvrum JakobOvrum commented on an outdated diff Jul 31, 2012
src/etc/linux/nullpointererror.d
+ * (See accompanying file LICENSE_1_0.txt)
+ * Authors: Amaury SECHET, FeepingCreature, Vladimir Panteleev
+ * Source: $(DRUNTIMESRC src/etc/linux/nullpointererror.d)
+ */
+module etc.linux.nullpointererror;
+
+version(linux) {
+
+private :
+import core.sys.posix.signal;
+import core.sys.posix.ucontext;
+
+// Missing details from Druntime
+
+version(X86_64) {
+ enum {
@JakobOvrum
D Programming Language member
JakobOvrum added a line comment Jul 31, 2012

Phobos uses 4 indendation levels.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JakobOvrum JakobOvrum commented on an outdated diff Jul 31, 2012
src/etc/linux/nullpointererror.d
@@ -0,0 +1,287 @@
+/**
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
+ * Note: Only x86 and x86_64 are supported for now.
+ *
+ * License: Distributed under the
+ * $(LINK2 http://www.boost.org/LICENSE_1_0.txt, Boost Software License 1.0).
+ * (See accompanying file LICENSE_1_0.txt)
+ * Authors: Amaury SECHET, FeepingCreature, Vladimir Panteleev
+ * Source: $(DRUNTIMESRC src/etc/linux/nullpointererror.d)
+ */
+module etc.linux.nullpointererror;
+
+version(linux) {
+
+private :
@JakobOvrum
D Programming Language member
JakobOvrum added a line comment Jul 31, 2012

The style guide does not cover this, but I'm fairly sure the convention is having no space between the private and the : tokens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JakobOvrum JakobOvrum commented on an outdated diff Jul 31, 2012
src/etc/linux/nullpointererror.d
+
+ // Save registers into global thread local, to allow recovery.
+ saved_RDI = context.uc_mcontext.gregs[REG_RDI];
+ saved_RSI = context.uc_mcontext.gregs[REG_RSI];
+
+ // Hijack current context so we call our handler.
+ auto rip = context.uc_mcontext.gregs[REG_RIP];
+ auto addr = cast(REG_TYPE) info.si_addr;
+ context.uc_mcontext.gregs[REG_RDI] = addr;
+ context.uc_mcontext.gregs[REG_RSI] = rip;
+ context.uc_mcontext.gregs[REG_RIP] = (rip != addr)?(cast(REG_TYPE) &sigsegv_data_handler):(cast(REG_TYPE) &sigsegv_code_handler);
+ }
+
+ // All handler functions must be called with faulting address in RDI and original RIP in RSI.
+
+ // This functionis called when the segfault's cause is to call an invalid function pointer.
@JakobOvrum
D Programming Language member
JakobOvrum added a line comment Jul 31, 2012

Missing space

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JakobOvrum JakobOvrum commented on an outdated diff Jul 31, 2012
src/etc/linux/nullpointererror.d
+ auto rip = context.uc_mcontext.gregs[REG_RIP];
+ auto addr = cast(REG_TYPE) info.si_addr;
+ context.uc_mcontext.gregs[REG_RDI] = addr;
+ context.uc_mcontext.gregs[REG_RSI] = rip;
+ context.uc_mcontext.gregs[REG_RIP] = (rip != addr)?(cast(REG_TYPE) &sigsegv_data_handler):(cast(REG_TYPE) &sigsegv_code_handler);
+ }
+
+ // All handler functions must be called with faulting address in RDI and original RIP in RSI.
+
+ // This functionis called when the segfault's cause is to call an invalid function pointer.
+ void sigsegv_code_handler() {
+ asm {
+ naked;
+
+ // Handle the stack for an invalid function call (segfault at RIP).
+ // With the return pointer, the stack is now alligned.
@JakobOvrum
D Programming Language member
JakobOvrum added a line comment Jul 31, 2012

aligned

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JakobOvrum JakobOvrum commented on an outdated diff Jul 31, 2012
src/etc/linux/nullpointererror.d
+ pop R11;
+ pop R10;
+ pop R9;
+ pop R8;
+ pop RDX;
+ pop RCX;
+ pop RAX;
+ popfq; // Restore flags.
+
+ // Return
+ pop RBP;
+ ret;
+ }
+ }
+
+ // The return value is stored in EAX and EDX, so this function restore the correct value for theses registers.
@JakobOvrum
D Programming Language member
JakobOvrum added a line comment Jul 31, 2012

restore -> restores

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JakobOvrum JakobOvrum commented on an outdated diff Jul 31, 2012
src/etc/linux/nullpointererror.d
+enum MEMORY_RESERVED_FOR_NULL_DEREFERENCE = 4096 * 16;
+
+// User space handler
+void sigsegv_userspace_process(void* address) {
+ // The first page is protected to detect null dereferences.
+ if((cast(size_t) address) < MEMORY_RESERVED_FOR_NULL_DEREFERENCE) {
+ throw new NullPointerError();
+ }
+
+ throw new InvalidPointerError();
+}
+
+public :
+
+/**
+ * Thrown on posix system when a signal is recieved. Is only throw for SIGSEGV.
@JakobOvrum
D Programming Language member
JakobOvrum added a line comment Jul 31, 2012

systems
thrown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JakobOvrum JakobOvrum commented on an outdated diff Jul 31, 2012
src/etc/linux/nullpointererror.d
+
+/**
+ * Thrown on posix system when a signal is recieved. Is only throw for SIGSEGV.
+ */
+class InvalidPointerError : Error {
+ this(string file = __FILE__, size_t line = __LINE__, Throwable next = null) {
+ super("", file, line, next);
+ }
+
+ this(Throwable next, string file = __FILE__, size_t line = __LINE__) {
+ super("", file, line, next);
+ }
+}
+
+/**
+ * Throw on null pointer dereferences.
@JakobOvrum
D Programming Language member
JakobOvrum added a line comment Jul 31, 2012

Thrown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@andralex
D Programming Language member

posix.mak seems to have conflicts.

An issue came up at work that brought about evidence that this may add significant practical value. In certain embedded uses (either statically or dynamically linked) it's important to avoid segfaulting. This module would go a long way toward making that possible.

@deadalnix, could you please bring this in shape (it's not rebased, won't build etc). Then let's merge it with experimental title. Thanks!

@deadalnix

@andralex What do you mean by merging with experimental title ? It is rebased and fixes has been made to solve conflicts.

@complexmath
D Programming Language member
@complexmath
D Programming Language member
@deadalnix

@complexmath Yes, it does jump into a subroutine after the signal handler complete.

It does work on linux (x86 and x86_64).

@andralex
D Programming Language member

I strongly think we should move forward with this. Any encumbrances left? Any wrinkles to iron out? If not, please rebase and let's get this puppy in the wild.

@andralex
D Programming Language member

Oh, it's already rebased. @complexmath: OK to merge?

@complexmath
D Programming Language member

Yep. GitHub is telling me this can't be automatically merged. Any idea why?

@alexrp
D Programming Language member

That just means we do need a rebase.

@alexrp alexrp and 1 other commented on an outdated diff Sep 2, 2012
src/etc/linux/memoryerror.d
+
+ // Restore register values and return.
+ call restore_registers;
+
+ pop ECX;
+ popfd; // Restore flags.
+
+ // Return
+ pop EBP;
+ ret;
+ }
+ }
+
+ // The return value is stored in EAX and EDX, so this function restore the correct value for theses registers.
+ REG_TYPE[2] restore_registers() {
+ return [saved_EAX, saved_EDX];
@alexrp
D Programming Language member
alexrp added a line comment Sep 2, 2012

This will result in a GC allocation. Just so you know. I don't know if it's a problem in this particular case.

@deadalnix
deadalnix added a line comment Sep 2, 2012

WTF I didn't knew that, but you are right, _d_arrayliteral is called and gc alloc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp alexrp and 4 others commented on an outdated diff Sep 2, 2012
src/etc/linux/memoryerror.d
+ REG_TYPE[2] restore_registers() {
+ return [saved_EAX, saved_EDX];
+ }
+}
+
+// This should be calculated by druntime.
+enum PAGE_SIZE = 4096;
+
+// The first 64Kb are reserved for detecting null pointer dereferencess.
+enum MEMORY_RESERVED_FOR_NULL_DEREFERENCE = 4096 * 16;
+
+// User space handler
+void sigsegv_userspace_process(void* address) {
+ // The first page is protected to detect null dereferences.
+ if((cast(size_t) address) < MEMORY_RESERVED_FOR_NULL_DEREFERENCE) {
+ throw new NullPointerError();
@alexrp
D Programming Language member
alexrp added a line comment Sep 2, 2012

I still don't agree with this. Dereferencing 0x1 is not dereferencing a null pointer. You are giving the programmer an incorrect error.

@complexmath
D Programming Language member
complexmath added a line comment Sep 2, 2012

The code still won't fire unless the system detected a segfault though, right?

@alexrp
D Programming Language member
alexrp added a line comment Sep 2, 2012

The code will trigger on all segmentation faults.

My gripe here is that the code throws NullPointerError for any dereference of a pointer in the range 0 - 64k. A null pointer is a null pointer. It is not 1, 2, 3, or any other integer value. Ever. The code should throw NullPointerError for 0 and 0 alone. Any other pointer should result in InvalidPointerError.

@andralex
D Programming Language member
andralex added a line comment Sep 2, 2012

@alexrp Actually accessing a field off the null pointer will trigger that. It's really same thing.

@klickverbot
D Programming Language member
klickverbot added a line comment Sep 2, 2012

@andralex: The point is that *(cast(ubyte*)0x100) = 1 will give you a NullPointerError, even if it's not a null pointer. I'm not sure whether there is any benefit to having NullPointerError along InvalidPointerError at all – it only seems to create confusion.

@deadalnix
deadalnix added a line comment Sep 3, 2012

Here is the reasoning :

As Andrei said, accessing a field of a null reference will trigger a page fault at address 0 + offset of the field. This is why linux kernel specifically reserve memory at 0 to detect null dereferences.

I see a point in discriminating null dereferences and other pointer errors. In the current state of D, you can dereference null really easily almost anywhere. Other memory errors are result of memory corruption.

The (cast(ubyte)0x100) = 1 is not a good example IMO because it is artificial. Pointer come from allocated memory, not constant that you cast. And no memory is ever allocated at 0 by the linux kernel.

As a result, you only trigger the NullPointerError on dereferencing null, or by building thing specifically to trigger that behavior, like @klickverbot did.

A NullPointerError is something you should expect to see on a regular basis, considering how D is done. Other memory error is something that must scare you very much because it means somewhere you have corrupted memory.

@alexrp
D Programming Language member
alexrp added a line comment Sep 3, 2012

Look... it doesn't matter where pointers come from. The point here is that this module's behavior is inconsistent.

Suppose I access some field inside a null object. The object's layout is small, so I get a NullPointerError. Now suppose I have a class with a very large (several megabytes) static array inside it. I access the last element of this array on a null object. Now I get an InvalidMemoryError.

Can you not see why this is a can of worms and will just cause confusion?

Not to mention, you can't maintain the same behavior across OSs - this reserved area is going to vary, harming future extension of this module!!

This module is supposed to provide a reliable mechanism to deal with invalid memory accesses, but it doesn't, until this stuff is fixed to be less Linux- and high-level-view-centric so it's actually future-proof and has a well-defined interface.

Just to be clear, I have nothing against this module and what it seeks to do. But I have absolutely everything against this one line of code that likes to pretend that assert(cast(void*)0x1 == null) passes.

@andralex
D Programming Language member
andralex added a line comment Sep 3, 2012

I think it's safe to drop this. IMHO objects with large embedded arrays should be, in fact, disallowed statically. Or if they're not, we don't guarantee integrity.

@alexrp
D Programming Language member
alexrp added a line comment Sep 3, 2012

@andralex wait, what? Why?

@andralex
D Programming Language member
andralex added a line comment Sep 3, 2012

We should cater for the common case, and the common case is that someone writes e.g.

if (object.field == 42) { ... }

Throwing a sort of an array dereference exception would be just confusing.

@klickverbot
D Programming Language member
klickverbot added a line comment Sep 3, 2012

@alexrp: Think about the implications concerning @safe-ty – you can read from an arbitrary memory location in safe code via dereferencing a null pointer to an object containing such a humongous array.

@alexrp
D Programming Language member
alexrp added a line comment Sep 3, 2012

Throwing a sort of an array dereference exception would be just confusing.

That's not what I'm saying we should... I'm saying that throwing NullPointerError is inconsistent, unreliable, and confusing. I say this a systems programmer who knows what the resulting assembly actually does. As such, I want an InvalidMemoryError because I am not dereferencing anything that is the value 0x0.

@andralex
D Programming Language member
andralex added a line comment Sep 3, 2012

@alexrp: are you saying you just want a change of name? (1) I'm glad this is the only thing that's under debate! (2) Although I understand your argument, I personally think NullPointerError is better than InvalidMemoryError.

@klickverbot
D Programming Language member
klickverbot added a line comment Sep 3, 2012

@andralex: The proposal already includes InvalidPointerError, but in addition a NullPointerError subclass which is thrown if the faulty address was particularly low.

@alexrp
D Programming Language member
alexrp added a line comment Sep 3, 2012

Just look at the code I commented on to begin with. I want that check to check for null and nothing else. That's all I'm asking for.

I'm willing to 'compromise' and just have InvalidPointerError for all cases (so no special-casing for close-to-null values) too.

@alexrp
D Programming Language member
alexrp added a line comment Sep 3, 2012

After some discussion on IRC, I have another proposal: Drop NullPointerError entirely and just attach RIP and the memory location that was attempted accessed (this information is trivially available across pretty much all relevant OS/arch combinations) to InvalidMemoryError and call it a day. You'll get a nice exception whenever you do an invalid memory access, and you can trivially ask (both programmatically and in a debugger) whether it's a null access.

Everyone wins?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp alexrp commented on an outdated diff Sep 2, 2012
src/etc/linux/memoryerror.d
+enum MEMORY_RESERVED_FOR_NULL_DEREFERENCE = 4096 * 16;
+
+// User space handler
+void sigsegv_userspace_process(void* address) {
+ // The first page is protected to detect null dereferences.
+ if((cast(size_t) address) < MEMORY_RESERVED_FOR_NULL_DEREFERENCE) {
+ throw new NullPointerError();
+ }
+
+ throw new InvalidPointerError();
+}
+
+public :
+
+/**
+ * Thrown on posix system when a signal is recieved. Is only throw for SIGSEGV.
@alexrp
D Programming Language member
alexrp added a line comment Sep 2, 2012

"Thrown on POSIX systems when a SIGSEGV signal is received."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp alexrp commented on an outdated diff Sep 2, 2012
src/etc/linux/memoryerror.d
+
+/**
+ * Thrown on posix system when a signal is recieved. Is only throw for SIGSEGV.
+ */
+class InvalidPointerError : Error {
+ this(string file = __FILE__, size_t line = __LINE__, Throwable next = null) {
+ super("", file, line, next);
+ }
+
+ this(Throwable next, string file = __FILE__, size_t line = __LINE__) {
+ super("", file, line, next);
+ }
+}
+
+/**
+ * Throw on null pointer dereferences.
@alexrp
D Programming Language member
alexrp added a line comment Sep 2, 2012

"Thrown on null pointer dereferences."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp alexrp commented on an outdated diff Sep 2, 2012
src/etc/linux/memoryerror.d
+
+ // Save registers into global thread local, to allow recovery.
+ saved_EAX = context.uc_mcontext.gregs[REG_EAX];
+ saved_EDX = context.uc_mcontext.gregs[REG_EDX];
+
+ // Hijack current context so we call our handler.
+ auto eip = context.uc_mcontext.gregs[REG_EIP];
+ auto addr = cast(REG_TYPE) info.si_addr;
+ context.uc_mcontext.gregs[REG_EAX] = addr;
+ context.uc_mcontext.gregs[REG_EDX] = eip;
+ context.uc_mcontext.gregs[REG_EIP] = (eip != addr)?(cast(REG_TYPE) &sigsegv_code_handler + 0x03):(cast(REG_TYPE) &sigsegv_data_handler);
+ }
+
+ // All handler functions must be called with faulting address in EAX and original EIP in EDX.
+
+ // This functionis called when the segfault's cause is to call an invalid function pointer.
@alexrp
D Programming Language member
alexrp added a line comment Sep 2, 2012

"function is"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@deadalnix

Rebased and everything.

@MartinNowak
D Programming Language member

Rebased and everything.

Nice.

Some final nitpicks

  • win32.mak MANIFEST and import is missing, see xattr.d
  • offering a register/unregister function instead of using shared static this would be more flexible
  • it seems that the REG_ enums are redefined in glibc, so we could put them in core.sys.linux.sys.ucontext
  • please untabify and delete-trailing-whitespace
  • most of druntime is written with opening braces on a separate line, so if you don't mind it would be good to keep this for new code
@jmdavis
D Programming Language member

most of druntime is written with opening braces on a separate line, so if you don't mind it would be good to keep this for new code

All druntime and Phobos code needs to follow this rule. If any code doesn't, it's because it's older code which hasn't been fixed, or it managed to slip passed the reviewers. It's one of the few formatting rules that we require (the major one other being the number of characters per line - soft limit of 80, hard limit of 120).

@alexrp
D Programming Language member

I think that it would be a good idea to attach some extra information to the errors this module throws. Specifically, I'd like the instruction address which triggered the error and the specific memory location that the instruction tried to access.

@deadalnix

@dawgfoto can you explain me what should I modify into win32.mak ? I don't really gt it, because this code is not usable in any way on windows.

I have made all other requested changes have been made except register/unregister .

@MartinNowak
D Programming Language member

@dawgfoto can you explain me what should I modify into win32.mak ? I don't really gt it, because this code is not usable in any way on windows.

Because Walter and/or other people use Windows to create the distribution files (dmd.zip).

@MartinNowak
D Programming Language member

We're really close, but please get rid of the shared static this() import initialization trick.
There are good use cases to only set a handler for a limited region of code. With a hardcoded initialization one cannot do this. You might also want to avoid the conflict with the unittest handler.

@deadalnix

@dawgfoto Is it correct now in regard of win32.mak and win64.mak ?

The shared this method has been replaced with register and unregister.

As the unittest signal handler does unregister itself, it shouldn't be a problem.

@alexrp alexrp commented on an outdated diff Sep 25, 2012
src/core/sys/posix/ucontext.d
@@ -39,6 +39,32 @@ version( linux )
version( X86_64 )
{
+ enum {
@alexrp
D Programming Language member
alexrp added a line comment Sep 25, 2012

Brace on separate line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp alexrp commented on an outdated diff Sep 25, 2012
src/core/sys/posix/ucontext.d
@@ -94,6 +120,28 @@ version( linux )
}
else version( X86 )
{
+ enum {
@alexrp
D Programming Language member
alexrp added a line comment Sep 25, 2012

Ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp alexrp commented on an outdated diff Sep 25, 2012
src/etc/linux/memoryerror.d
+
+ return sigaction(SIGSEGV, &action, oldptr);
+}
+
+int unregister_memory_error_handler()
+{
+ auto oldptr = cast(sigaction_t*) &old_sigaction;
+
+ return sigaction(SIGSEGV, oldptr, null);
+}
+
+// Sighandler space
+
+alias typeof({ucontext_t uc; return uc.uc_mcontext.gregs[0];}()) REG_TYPE;
+
+version(X86_64) {
@alexrp
D Programming Language member
alexrp added a line comment Sep 25, 2012

Brace on separate line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp alexrp commented on an outdated diff Sep 25, 2012
src/etc/linux/memoryerror.d
+ {
+ asm {
+ naked;
+
+ // Handle the stack for an invalid function call (segfault at RIP).
+ // With the return pointer, the stack is now alligned.
+ push RBP;
+ mov RBP, RSP;
+
+ jmp sigsegv_data_handler;
+ }
+ }
+
+ void sigsegv_data_handler()
+ {
+ asm {
@alexrp
D Programming Language member
alexrp added a line comment Sep 25, 2012

Ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp alexrp commented on an outdated diff Sep 25, 2012
src/etc/linux/memoryerror.d
+ saved_RSI = context.uc_mcontext.gregs[REG_RSI];
+
+ // Hijack current context so we call our handler.
+ auto rip = context.uc_mcontext.gregs[REG_RIP];
+ auto addr = cast(REG_TYPE) info.si_addr;
+ context.uc_mcontext.gregs[REG_RDI] = addr;
+ context.uc_mcontext.gregs[REG_RSI] = rip;
+ context.uc_mcontext.gregs[REG_RIP] = (rip != addr)?(cast(REG_TYPE) &sigsegv_data_handler):(cast(REG_TYPE) &sigsegv_code_handler);
+ }
+
+ // All handler functions must be called with faulting address in RDI and original RIP in RSI.
+
+ // This function is called when the segfault's cause is to call an invalid function pointer.
+ void sigsegv_code_handler()
+ {
+ asm {
@alexrp
D Programming Language member
alexrp added a line comment Sep 25, 2012

Ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp alexrp and 1 other commented on an outdated diff Sep 25, 2012
src/etc/linux/memoryerror.d
+ pop RBP;
+ ret;
+ }
+ }
+
+ // The return value is stored in EAX and EDX, so this function restore the correct value for theses registers.
+ REG_TYPE restore_RDI()
+ {
+ return saved_RDI;
+ }
+
+ REG_TYPE restore_RSI()
+ {
+ return saved_RSI;
+ }
+} else version(X86) {
@alexrp
D Programming Language member
alexrp added a line comment Sep 25, 2012

Ditto (x2).

@andralex
D Programming Language member
andralex added a line comment Sep 25, 2012

@alexrp to save yourself work just write "throughout" the second time around and don't mention it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp alexrp commented on an outdated diff Sep 25, 2012
src/etc/linux/memoryerror.d
+ saved_EDX = context.uc_mcontext.gregs[REG_EDX];
+
+ // Hijack current context so we call our handler.
+ auto eip = context.uc_mcontext.gregs[REG_EIP];
+ auto addr = cast(REG_TYPE) info.si_addr;
+ context.uc_mcontext.gregs[REG_EAX] = addr;
+ context.uc_mcontext.gregs[REG_EDX] = eip;
+ context.uc_mcontext.gregs[REG_EIP] = (eip != addr)?(cast(REG_TYPE) &sigsegv_code_handler + 0x03):(cast(REG_TYPE) &sigsegv_data_handler);
+ }
+
+ // All handler functions must be called with faulting address in EAX and original EIP in EDX.
+
+ // This function is called when the segfault's cause is to call an invalid function pointer.
+ void sigsegv_code_handler()
+ {
+ asm {
@alexrp
D Programming Language member
alexrp added a line comment Sep 25, 2012

Ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp alexrp commented on an outdated diff Sep 25, 2012
src/etc/linux/memoryerror.d
+ asm {
+ naked;
+
+ // Handle the stack for an invalid function call (segfault at EIP).
+ // 4 bytes are used for function pointer; We need 12 byte to keep stack aligned.
+ sub ESP, 12;
+ mov 8[ESP], EBP;
+ mov EBP, ESP;
+
+ jmp sigsegv_data_handler;
+ }
+ }
+
+ void sigsegv_data_handler()
+ {
+ asm {
@alexrp
D Programming Language member
alexrp added a line comment Sep 25, 2012

Ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp
D Programming Language member

@deadalnix The makefile changes look correct to me.

@alexrp
D Programming Language member

@deadalnix Actually, this is failing on Windows: http://d.puremagic.com/test-results/pull.ghtml?runid=300885

You shouldn't add the module to the list of modules to build on Windows (SRCS), just the list of modules to zip (MANIFEST).

@deadalnix

@alexrp new line added and module removed from SRCS on windows.

@MartinNowak MartinNowak was assigned Sep 25, 2012
@andralex
D Programming Language member

I'm assigning this to @dawgfoto. Thanks all!

@andralex
D Programming Language member

What's the deal with the Win32 build error?

@JakobOvrum JakobOvrum commented on an outdated diff Oct 24, 2012
src/etc/linux/memoryerror.d
@@ -0,0 +1,277 @@
+/**
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
@JakobOvrum
D Programming Language member
JakobOvrum added a line comment Oct 24, 2012

Some corrections:
errors
thrown (twice)
dereferencing
system-dependent

It's also a good idea to use $(D symbol) when referencing symbols.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JakobOvrum JakobOvrum commented on an outdated diff Oct 24, 2012
src/etc/linux/memoryerror.d
+/**
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
+ * Note: Only x86 and x86_64 are supported for now.
+ *
+ * License: Distributed under the
+ * $(LINK2 http://www.boost.org/LICENSE_1_0.txt, Boost Software License 1.0).
+ * (See accompanying file LICENSE_1_0.txt)
+ * Authors: Amaury SECHET, FeepingCreature, Vladimir Panteleev
+ * Source: $(DRUNTIMESRC src/etc/linux/memory.d)
+ */
+module etc.linux.memoryerror;
+
+version(linux)
+{
+
+private :
@JakobOvrum
D Programming Language member
JakobOvrum added a line comment Oct 24, 2012

The Phobos code I've seen does not put a space between the two tokens in code like this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexrp
D Programming Language member

@deadalnix can you address @JakobOvrum's points?

@MartinNowak MartinNowak commented on an outdated diff Nov 18, 2012
src/etc/linux/memoryerror.d
+ * $(LINK2 http://www.boost.org/LICENSE_1_0.txt, Boost Software License 1.0).
+ * (See accompanying file LICENSE_1_0.txt)
+ * Authors: Amaury SECHET, FeepingCreature, Vladimir Panteleev
+ * Source: $(DRUNTIMESRC src/etc/linux/memory.d)
+ */
+module etc.linux.memoryerror;
+
+version(linux)
+{
+
+private :
+import core.sys.posix.signal;
+import core.sys.posix.ucontext;
+
+// Register and unregister memory error handler.
+private shared sigaction_t old_sigaction;
@MartinNowak
D Programming Language member
MartinNowak added a line comment Nov 18, 2012

You might want to use __gshared here to avoid casting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MartinNowak MartinNowak commented on an outdated diff Nov 18, 2012
src/etc/linux/memoryerror.d
+
+ auto oldptr = cast(sigaction_t*) &old_sigaction;
+
+ return sigaction(SIGSEGV, &action, oldptr);
+}
+
+int unregister_memory_error_handler()
+{
+ auto oldptr = cast(sigaction_t*) &old_sigaction;
+
+ return sigaction(SIGSEGV, oldptr, null);
+}
+
+// Sighandler space
+
+alias typeof({ucontext_t uc; return uc.uc_mcontext.gregs[0];}()) REG_TYPE;
@MartinNowak
D Programming Language member
MartinNowak added a line comment Nov 18, 2012

You can use the init property to abbreviate the alias.
alias typeof(ucontext_t.init.uc_mcontext.gregs[0]) REG_TYPE;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MartinNowak MartinNowak commented on an outdated diff Nov 18, 2012
src/etc/linux/memoryerror.d
+
+ extern(C)
+ void handleSignal(int signum, siginfo_t* info, void* contextPtr)
+ {
+ auto context = cast(ucontext_t*)contextPtr;
+
+ // Save registers into global thread local, to allow recovery.
+ saved_RDI = context.uc_mcontext.gregs[REG_RDI];
+ saved_RSI = context.uc_mcontext.gregs[REG_RSI];
+
+ // Hijack current context so we call our handler.
+ auto rip = context.uc_mcontext.gregs[REG_RIP];
+ auto addr = cast(REG_TYPE) info.si_addr;
+ context.uc_mcontext.gregs[REG_RDI] = addr;
+ context.uc_mcontext.gregs[REG_RSI] = rip;
+ context.uc_mcontext.gregs[REG_RIP] = (rip != addr)?(cast(REG_TYPE) &sigsegv_data_handler):(cast(REG_TYPE) &sigsegv_code_handler);
@MartinNowak
D Programming Language member
MartinNowak added a line comment Nov 18, 2012

The line length is 129!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MartinNowak MartinNowak commented on an outdated diff Nov 18, 2012
src/etc/linux/memoryerror.d
+
+ extern(C)
+ void handleSignal(int signum, siginfo_t* info, void* contextPtr)
+ {
+ auto context = cast(ucontext_t*)contextPtr;
+
+ // Save registers into global thread local, to allow recovery.
+ saved_EAX = context.uc_mcontext.gregs[REG_EAX];
+ saved_EDX = context.uc_mcontext.gregs[REG_EDX];
+
+ // Hijack current context so we call our handler.
+ auto eip = context.uc_mcontext.gregs[REG_EIP];
+ auto addr = cast(REG_TYPE) info.si_addr;
+ context.uc_mcontext.gregs[REG_EAX] = addr;
+ context.uc_mcontext.gregs[REG_EDX] = eip;
+ context.uc_mcontext.gregs[REG_EIP] = (eip != addr)?(cast(REG_TYPE) &sigsegv_code_handler + 0x03):(cast(REG_TYPE) &sigsegv_data_handler);
@MartinNowak
D Programming Language member
MartinNowak added a line comment Nov 18, 2012

line length

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MartinNowak
D Programming Language member

Please stick to camel casing for function names.

@deadalnix

What is the hard limit for line length ? What is the policy to split ternary operator on multiple line ?

@jmdavis
D Programming Language member

What is the hard limit for line length ?

There is a soft character limit of 80 characters per line and a hard limit of 120. So, most lines should be within 80 characters but no lines can exceed 120.

What is the policy to split ternary operator on multiple line ?

There is none. For the most part, we don't have much in the way of style rules about formatting. They boil down primarily to using 4 spaces per indent (no tabs) and putting braces on their own line. For most of the rest, it's primarily a matter of making sure that the style in the file is reasonably consistent, and there probably aren't enough ternary operators being used to even set a precedent within most files. Personally, if the line with a ternary operator is too long, I do

auto result = condition
              ? branch1
              : branch2

but I expect that there's code in druntime and/or Phobos which formats that differently. At minimum, if Andrei were doing it, he'd only indent the 2nd and 3rd lines by 4 spaces rather than lining them up with the previous line. It all depends on who wrote the code and what file it's in (as the formatting style varies from file to file).

@MartinNowak MartinNowak merged commit c5b30a5 into dlang:master Nov 29, 2012

1 check failed

Details default Pass: 4, Fail: 2, Pending: 4
@MartinNowak
D Programming Language member

OK let's give it a try.
I've added the missing win{32,64}.mak rules here and here.

@andralex
D Programming Language member

historical moment!

@deadalnix

This one have been epic :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment