Very lost on botched interrupts causing triple fault

[Octocontrabass]

Then the answer is yes, you did set the interrupt flag.

Are you receiving IRQs from any hardware other than the keyboard controller? For example, the timer is probably configured to raise IRQ0.

[austanss]

Then the answer is yes, you did set the interrupt flag.

Are you receiving IRQs from any hardware other than the keyboard controller? For example, the timer is probably configured to raise IRQ0.

My IRQ handler is one function, that takes an argument of interrupt code. I don't really understand what you mean... I'm not getting any notifications of interrupts like I programmed it to.

[Octocontrabass]

Does your IRQ handler get called at all? Try running it in a debugger. You'll be able to set a breakpoint to see.

[austanss]

Does your IRQ handler get called at all? Try running it in a debugger. You'll be able to set a breakpoint to see.

I don't know how I would run it in a debugger, and fwiw, some jackass stripped the build of it's debug symbols.

[austanss]

ISR.asm

Code: Select all

%macro ISRWithoutErrorCode 1
  global isr%1
  isr%1:
        cli
        push byte 0
        push byte %1
        jmp CommonISRStub
%endmacro

%macro ISRWithErrorCode 1
  global isr%1
  isr%1:
        cli
        push byte %1
        jmp CommonISRStub
%endmacro

ISRWithoutErrorCode 0
ISRWithoutErrorCode 1
ISRWithoutErrorCode 2
ISRWithoutErrorCode 3
ISRWithoutErrorCode 4
ISRWithoutErrorCode 5
ISRWithoutErrorCode 6
ISRWithErrorCode 7
ISRWithoutErrorCode 8
ISRWithErrorCode 9
ISRWithErrorCode 10
ISRWithErrorCode 11
ISRWithErrorCode 12
ISRWithErrorCode 13
ISRWithoutErrorCode 14
ISRWithoutErrorCode 15
ISRWithoutErrorCode 16
ISRWithoutErrorCode 17
ISRWithoutErrorCode 18
ISRWithoutErrorCode 19
ISRWithoutErrorCode 20
ISRWithoutErrorCode 21
ISRWithoutErrorCode 22
ISRWithoutErrorCode 23
ISRWithoutErrorCode 24
ISRWithoutErrorCode 25
ISRWithoutErrorCode 26
ISRWithoutErrorCode 27
ISRWithoutErrorCode 28
ISRWithoutErrorCode 29
ISRWithoutErrorCode 30
ISRWithoutErrorCode 31

EXTERN ISRHandler

CommonISRStub:
        pusha

        mov ax, ds
        push eax

        mov ax, 0x10
        mov ds, ax
        mov es, ax
        mov fs, ax
        mov gs, ax

        call ISRHandler

        pop eax
        mov ds, ax
        mov es, ax
        mov fs, ax
        mov gs, ax

        popa
        add esp, 8
        sti
        iret

from GDT.cpp

Code: Select all

struct Registers
{
        uint32_t ds;
        uint32_t edi, esi, ebp, esp, ebx, edx, ecx, eax;
        uint32_t interruptNumber, errorCode;
        uint32_t eip, cs, eflags, useresp, ss;
};


void ISRHandlerImpl(Registers& registers)
{
        if (registers.interruptNumber == 1 || registers.interruptNumber == 33) {
                auto keycode = inb(0x60);

                if (keycode < 0) {
                        return;
                }

                Terminal::instance->write(keycode);
                Terminal::instance->write("\n");
        }
        Terminal::instance->write("Interrupt: ");
        Terminal::instance->write(registers.interruptNumber);
        Terminal::instance->write("\n");
}

void ISRHandler(Registers registers)
{
        outb(0xA0, 0x20);
        ISRHandlerImpl(registers);
        outb(0x20, 0x20);
}

[MichaelPetch]

You need to add entries to your IDT for the IRQs from 0x20 to 0x2f (32 to 47). Right now your interrupts aren't being redirected into a handler like the ISRs you already have. As well your going to run into problems if you build with optimizations. `CommonISRStub` builds the register structure by value and not by reference. This can cause the compiler to generate code that can corrupt the saved interrupt stack. `CommonISRStub` should push the address of the register structure as the first parameter to ISRHandler. Something like:

Code: Select all

CommonISRStub:
        pusha

        mov ax, ds
        push eax

        mov ax, 0x10
        mov ds, ax
        mov es, ax
        mov fs, ax
        mov gs, ax

        cld
        push esp
        call ISRHandler

        pop eax
        pop eax
        mov ds, ax
        mov es, ax
        mov fs, ax
        mov gs, ax

        popa
        add esp, 8
;       sti
        iret

Then ISRHandler should be defined this way:

Code: Select all

void ISRHandler(Registers& registers)

instead of

Code: Select all

void ISRHandler(Registers registers)

.

You also don't handle the EOIs properly in ISRHandler. You shouldn't put an STI at the end of CommonISRStub. And one serious concern I have is that your Linker script and startup code doesn't call the global constructors (the constructors of objects at global scope). This could cause serious problems for any objects that were supposed to have constructors initialize objects.

[austanss]

You need to add entries to your IDT for the IRQs from 0x20 to 0x2f (32 to 47). Right now your interrupts aren't being redirected into a handler like the ISRs you already have. As well your going to run into problems if you build with optimizations. `CommonISRStub` builds the register structure by value and not by reference. This can cause the compiler to generate code that can corrupt the saved interrupt stack. `CommonISRStub` should push the address of the register structure as the first parameter to ISRHandler. Something like:

Code: Select all

CommonISRStub:
        pusha

        mov ax, ds
        push eax

        mov ax, 0x10
        mov ds, ax
        mov es, ax
        mov fs, ax
        mov gs, ax

        cld
        push esp
        call ISRHandler

        pop eax
        pop eax
        mov ds, ax
        mov es, ax
        mov fs, ax
        mov gs, ax

        popa
        add esp, 8
;       sti
        iret

Then ISRHandler should be defined this way:

Code: Select all

void ISRHandler(Registers& registers)

instead of

Code: Select all

void ISRHandler(Registers registers)

.

You also don't handle the EOIs properly in ISRHandler. You shouldn't put an STI at the end of CommonISRStub. And one serious concern I have is that your Linker script and startup code doesn't call the global constructors (the constructors of objects at global scope). This could cause serious problems for any objects that were supposed to have constructors initialize objects.

I don't have any global constructors...

[MichaelPetch]

I should have looked closer at the code. I made a false assumption looking at your GDT.cpp file and boot.S. I assumed that there is a global Terminal object in your system but you create it on the stack inside kernel_main. My assumption that there was a global `Terminal` that was already initialized was because in boot.S you call `loadGDT` that will setup the ISRs (and eventually a proper one for the timer and the keyboard), and then before you call `kernel_main` from boot.S you enable interrupts with `STI`.

If someone were to hit a key on the keyboard after you did `STI` (in boot.S) and before the constructor of `Terminal` is called in `kernel_main` then this code in ISRHandlerImpl:

Code: Select all

Terminal::instance->write(keycode);

will attempt to use `instance` before it has been set to anything which could cause problems.

Usually for a C++ singleton pattern most people create something like a static `instance()` function inside the class which checks to see if the static instance variable in the object is nullptr and if it is it internally creates a new copy of the object and returns that new pointer (and sets the instance variable with it) otherwise it just returns the instance variable. There is of course a cleaner way and that is for the `instance()` function to simply create a static object of itself and return that object reference. Ultimately you call `instance()` this way:

Code: Select all

Terminal::instance()->write(keycode);

This way you would be guaranteed to have a valid instance returned. This does have the disadvantage of potentially having more overhead on each call.

[austanss]

I should have looked closer at the code. I made a false assumption looking at your GDT.cpp file and boot.S. I assumed that there is a global Terminal object in your system but you create it on the stack inside kernel_main. My assumption that there was a global `Terminal` that was already initialized was because in boot.S you call `loadGDT` that will setup the ISRs (and eventually a proper one for the timer and the keyboard), and then before you call `kernel_main` from boot.S you enable interrupts with `STI`.

If someone were to hit a key on the keyboard after you did `STI` (in boot.S) and before the constructor of `Terminal` is called in `kernel_main` then this code in ISRHandlerImpl:

Code: Select all

Terminal::instance->write(keycode);

will attempt to use `instance` before it has been set to anything which could cause problems.

Usually for a C++ singleton pattern most people create something like a static `instance()` function inside the class which checks to see if the static instance variable in the object is nullptr and if it is it internally creates a new copy of the object and returns that new pointer (and sets the instance variable with it) otherwise it just returns the instance variable. There is of course a cleaner way and that is for the `instance()` function to simply create a static object of itself and return that object reference. Ultimately you call `instance()` this way:

Code: Select all

Terminal::instance()->write(keycode);

This way you would be guaranteed to have a valid instance returned. This does have the disadvantage of potentially having more overhead on each call.

Would it also be possible, since kernel_main does stack-allocate the Terminal instance, that the Terminal instance is being destroyed after kernel_main ends? Or do we not have luxuries like that in freestanding environments?

[Octocontrabass]

Would it also be possible, since kernel_main does stack-allocate the Terminal instance, that the Terminal instance is being destroyed after kernel_main ends?

Yes.

[MichaelPetch]

Would it also be possible, since kernel_main does stack-allocate the Terminal instance, that the Terminal instance is being destroyed after kernel_main ends? Or do we not have luxuries like that in freestanding environments?

The object on the stack would go out of scope as soon as `kernel_main` returns. Which means the destructor will be called and the space occupied by that object on the stack will be released. You won't be able to rely on any of the non static member variables in that object being valid anymore nor will the instance pointer be pointing to a valid object anymore (it would be pointing at the one that was on the stack). That will pose a problem in your code since you don't put an endless `hlt` loop in your `kernel_main` but let it return back to Boot.S to do the loop there.

There is a good chance that if you got interrupts going that and you don't fix the Terminal object and how you use it you may not see the output you are expecting in the ISRHandlerImpl (or it may just crash).

[austanss]

Would it also be possible, since kernel_main does stack-allocate the Terminal instance, that the Terminal instance is being destroyed after kernel_main ends? Or do we not have luxuries like that in freestanding environments?

The object on the stack would go out of scope as soon as `kernel_main` returns. Which means the destructor will be called and the space occupied by that object on the stack will be released. You won't be able to rely on any of the non static member variables in that object being valid anymore nor will the instance pointer be pointing to a valid object anymore (it would be pointing at the one that was on the stack). That will pose a problem in your code since you don't put an endless `hlt` loop in your `kernel_main` but let it return back to Boot.S to do the loop there.

There is a good chance that if you got interrupts going that and you don't fix the Terminal object and how you use it you may not see the output you are expecting in the ISRHandlerImpl (or it may just crash).

I figured that would pose a problem, I just was unsure, and wasn't going to touch code I didn't write if I was unsure.

[MichaelPetch]

The quickest thing I can think of is remove the instance variable from you Terminal class altogether. Define your Terminal class this way:

Code: Select all

#pragma once

#include "stddef.h"
#include "stdint.h"

class Terminal
{
        static const size_t VGA_WIDTH = 80;
        static const size_t VGA_HEIGHT = 25;

        size_t row;
        size_t column;
        uint16_t* buffer;

public:
        static constexpr const char* Status = "$WHITE![$LIGHT_BLUE!--$WHITE!] $LIGHT_GREY!\0";
        static constexpr const char* Good = "$WHITE![$LIGHT_GREEN!:)$WHITE!] $LIGHT_GREY!\0";
        static constexpr const char* EOL = "\n";
        static Terminal &instance();

        void put_entry_at(char c, uint8_t color, size_t x, size_t y);
        void put_char(char c, uint8_t color);
        void write(const char* data, size_t size);
        void write(const char* data);
        void write(int num);
        void println(const char* data = "");
        void shift();

private:
        Terminal();
        Terminal(Terminal const&);
        void operator=(Terminal const&);
};

template<typename T>
Terminal& operator<<(Terminal& term, T data)
{
        term.write(data);
        return term;
}

template<typename T>
Terminal* operator<<(Terminal* term, T data)
{
        term->write(data);
        return term;
}

I have deleted the `instance` variable (in the header and in the Terminal.cpp file) and created an `instance` function that returns a Terminal reference. I have also made the constructor private so that you can't create a Terminal object directly. In your Terminal.cpp you can add this new instance function:

Code: Select all

Terminal &Terminal::instance()
{
        static Terminal instance;

        return instance;
}

This has a static `Terminal` object called 'instance' in it. We just return a reference to that object to the caller. The instance() function will take care of initializing `instance` by calling its constructor if needed. This is what produces a singleton object. To use it in Kernel.cpp you'd do something like:

Code: Select all

void kernel_main()
{
        Terminal &terminal = Terminal::instance();
        terminal << logo;
        terminal << Terminal::EOL;

        // rest of code here
}

In your ISRHandlerImpl you could use it like:

Code: Select all

                Terminal::instance().write(keycode);
                Terminal::instance().write("\n");

Depending on your compiler you may also have to add the option `'-fno-threadsafe-statics'` to meson.build compile arguments. This is because many compilers will produce thread safe code around the static local object ('instance' in this case). This could generate calls to functions you haven't implemented that maintain thread safe access to these objects. Implementing the required functions to maintain thread safety would be the best course of action in the long term. There is a page somewhere on the OSDev Wiki about the functions that need to be implemented.

[MichaelPetch]

I don't know how I would run it in a debugger, and fwiw, some jackass stripped the build of it's debug symbols.

. I build with

Code: Select all

env CXX=clang++ CC=clang meson _build --buildtype debug

And I debug with QEMU and GDB with something like:

Code: Select all

qemu-system-i386 -cdrom _build/NovaVita.iso -S -s -no-reboot -no-shutdown &

gdb ./_build/NovaCor/Kernel/NovaVita.kernel.debug \
        -ex 'target remote localhost:1234' \
        -ex 'layout src' \
        -ex 'layout regs' \
        -ex 'break _start' \
        -ex 'continue'

You can set a start breakpoint anywhere. You could make it `kernel_main` instead of `_start`. While debugging if you need to debug assembly code you can use `layout asm` at the GDB prompt. You can find tutorials and cheat sheets online for more detailed GDB commands once the debugger is running.

[austanss]

I have determined the cause of my lack of interrupt functionality. The interrupts are being handled, however due to the piece of **** of a terminal class I have, the terminal doesn't display them. Thank you everyone, for your support. I can work on the terminal myself, however.