7
\$\begingroup\$

I learn about OS development and follow the tutorial Bare Bones on OSDev. And I did some extra tasks from the Moving Forward section.

I would like to get any suggestion about code improvement, especially about the Makefile and the project layout on GitHub.

kernel.c:

#include "string.h"
#include "vga.h"
#include "terminal.h"

void KernelMain(void) 
{
    Terminal_Init();

    Terminal_SetColor(Vga_CreateEntryColor(VGA_COLOR_LIGHT_BLUE, VGA_COLOR_BLACK));
    Terminal_Write("Hello, World!");
}

string.h:

#ifndef STRING_H
#define STRING_H

#include <stddef.h>

size_t strlen(const char *);

#endif

string.c:

#include "string.h"

/*
===============
strlen
===============
*/
size_t strlen(const char* s)
{
    size_t length = 0;

    while (s[length])
        length++;

    return length;
}

vga.h:

#ifndef VGA_H
#define VGA_H

#include <stdint.h>
#include <stddef.h>

#define VGA_TEXT_MODE_BUFFER_ADDRESS ((VgaEntryChar *) 0xB8000)
#define VGA_WIDTH 80
#define VGA_HEIGHT 26

enum VgaColor {
    VGA_COLOR_BLACK,
    VGA_COLOR_BLUE,
    VGA_COLOR_GREEN,
    VGA_COLOR_CYAN,
    VGA_COLOR_RED,
    VGA_COLOR_MAGENTA,
    VGA_COLOR_BROWN,
    VGA_COLOR_LIGHT_GREY,
    VGA_COLOR_DARK_GREY,
    VGA_COLOR_LIGHT_BLUE,
    VGA_COLOR_LIGHT_GREEN,
    VGA_COLOR_LIGHT_CYAN,
    VGA_COLOR_LIGHT_RED,
    VGA_COLOR_LIGHT_MAGENTA,
    VGA_COLOR_LIGHT_BROWN,
    VGA_COLOR_WHITE
};

typedef uint8_t VgaEntryColor;
typedef uint16_t VgaEntryChar;

VgaEntryColor Vga_CreateEntryColor(enum VgaColor, enum VgaColor);
VgaEntryChar Vga_CreateEntryChar(char, VgaEntryColor);
void Vga_PutEntryChar(VgaEntryChar, size_t, size_t);
char Vga_GetCharacterFromEntryChar(VgaEntryChar);

#endif

vga.c:

#include "vga.h"

/*
===============
Vga_CreateEntryColor
===============
*/
VgaEntryColor Vga_CreateEntryColor(enum VgaColor fg, enum VgaColor bg)
{
    return fg | bg << 4;
}

/*
===============
Vga_CreateEntryChar
===============
*/
VgaEntryChar Vga_CreateEntryChar(char c, VgaEntryColor color)
{
    return (VgaEntryChar) c | (VgaEntryChar) color << 8;
}

/*
===============
Vga_PutEntryChar
===============
*/
void Vga_PutEntryChar(VgaEntryChar c, size_t x, size_t y)
{
    VGA_TEXT_MODE_BUFFER_ADDRESS[y * VGA_WIDTH + x] = c;
}

/*
===============
Vga_GetCharacterFromEntryChar
===============
*/
char Vga_GetCharacterFromEntryChar(VgaEntryChar c)
{
    return c & 0x00FF;
}

terminal.h:

#ifndef TERMINAL_H
#define TERMINAL_H

#include <stdint.h>
#include <stddef.h>
#include "vga.h"

/* Terminal width should always equals to VGA_WIDTH. */
#define TERMINAL_WIDTH VGA_WIDTH
#define TERMINAL_HEIGHT 1000

void Terminal_Init(void);
void Terminal_Write(const char *);
void Terminal_SetColor(VgaEntryColor);
void Terminal_PutChar(char);
void Terminal_Clear(void);

#endif

terminal.c:

#include "terminal.h"

struct Terminal {
    size_t row, column;
    VgaEntryColor color;
    VgaEntryChar backBuffer[TERMINAL_HEIGHT][TERMINAL_WIDTH];
} terminal = {
    0, 0,
    0,
    {{0}}
};

static void PutCharToBackBuffer(VgaEntryChar);
static void PutCharToBackBufferAtCoordinates(VgaEntryChar, size_t, size_t);
static void DisplayBackBuffer(void);
static VgaEntryChar GetCharFromBackBuffer(size_t, size_t);
static void ClearBackBuffer(void);
static void GoToNewLineInBackBuffer(void);

/*
===============
Terminal_Init
===============
*/
void Terminal_Init(void)
{
    Terminal_SetColor(Vga_CreateEntryColor(VGA_COLOR_LIGHT_GREY, VGA_COLOR_BLACK));
    Terminal_Clear();
}

/*
===============
Terminal_Clear
===============
*/
void Terminal_Clear(void)
{
    ClearBackBuffer();
    DisplayBackBuffer();
}

/*
===============
ClearBackBuffer
===============
*/
static void ClearBackBuffer(void)
{
    const VgaEntryChar c = Vga_CreateEntryChar(' ', Vga_CreateEntryColor(VGA_COLOR_LIGHT_GREY, VGA_COLOR_BLACK));

    for (size_t y = 0; y < TERMINAL_HEIGHT; ++y)
        for (size_t x = 0; x < TERMINAL_WIDTH; ++x)
            PutCharToBackBuffer(c);

    terminal.row = 0;
    terminal.column = 0;
}

/*
===============
PutCharToBackBuffer

Puts a colored character to the back buffer on the current cursor position.
If there is no line space left, the cursor moves to the new line. If there
is no vertical space, the cursor moves to the first line.
===============
*/
static void PutCharToBackBuffer(VgaEntryChar c)
{
    if (Vga_GetCharacterFromEntryChar(c) == '\n') {
        GoToNewLineInBackBuffer();
    } else {
        PutCharToBackBufferAtCoordinates(c, terminal.column, terminal.row);

        if (++terminal.column == TERMINAL_WIDTH)
            GoToNewLineInBackBuffer();
    }
}

/*
===============
GoToNewLineInBackBuffer
===============
*/
static void GoToNewLineInBackBuffer(void)
{
    terminal.column = 0;

    if (++terminal.row == TERMINAL_HEIGHT)
        terminal.row = 0;
}

/*
===============
PutCharToBackBufferAtCoordinates
===============
*/
static void PutCharToBackBufferAtCoordinates(VgaEntryChar c, size_t x, size_t y)
{
    terminal.backBuffer[y][x] = c;
}

/*
===============
DisplayBackBuffer
===============
*/
static void DisplayBackBuffer(void)
{
    const size_t offset = terminal.row > VGA_HEIGHT ? terminal.row - VGA_HEIGHT : 0;

    for (size_t y = 0; y < VGA_HEIGHT; ++y)
        for (size_t x = 0; x < VGA_WIDTH; ++x)
            Vga_PutEntryChar(GetCharFromBackBuffer(x, y + offset), x, y);
}

/*
===============
Terminal_SetColor
===============
*/
void Terminal_SetColor(VgaEntryColor c)
{
    terminal.color = c;
}

/*
===============
Terminal_Write
===============
*/
void Terminal_Write(const char *s)
{
    while (*s)
        Terminal_PutChar(*s++);

    DisplayBackBuffer();
}

/*
===============
Terminal_PutChar
===============
*/
void Terminal_PutChar(char c)
{
    PutCharToBackBuffer(Vga_CreateEntryChar(c, terminal.color));
    DisplayBackBuffer();
}

/*
===============
GetCharFromBackBuffer
===============
*/
static VgaEntryChar GetCharFromBackBuffer(size_t x, size_t y)
{
    return terminal.backBuffer[y][x];
}

Makefile:

#
# Compiler and assembler options
#

CC = i686-elf-gcc
AS = i686-elf-as
CFLAGS = -std=c99 -ffreestanding -nostdlib -O0 -Wall -Wextra -pedantic

#
# Project's hierarchy
#

SRCDIR = src
BINDIR = bin
BINARY = kernel.bin
ISODIR = isodir
RESULTING_ISO = kernel.iso

#
# Colored printing
#

BLUE_COLOR = $(shell tput setaf 4)
GREEN_COLOR = $(shell tput setaf 2)
RESET_COLOR = $(shell tput sgr0)
BOLD = $(shell tput bold)
OFFBOLD = $(shell tput rmso)
MESSAGE_BEGIN = ${BOLD}${GREEN_COLOR}
MESSAGE_END = ${RESET_COLOR}${OFFBOLD}

.PHONY: clean makeiso run

${BINDIR}/${BINARY}: boot.o kernel.o string.o vga.o terminal.o
    ${info }
    ${info ${MESSAGE_BEGIN}Binary is linking...${MESSAGE_END}}
    ${CC} -T linker.ld ${CFLAGS} $^ -o $@ -lgcc

boot.o: ${SRCDIR}/boot.s
    ${info ${MESSAGE_BEGIN}boot.s is compiling...${MESSAGE_END}}
    ${AS} $< -o $@

kernel.o: ${SRCDIR}/kernel.c
    ${info }
    ${info ${MESSAGE_BEGIN}kernel.c is compiling...${MESSAGE_END}}
    ${CC} ${CFLAGS} -c $< -o $@

string.o: ${SRCDIR}/string.c ${SRCDIR}/string.h
    ${info }
    ${info ${MESSAGE_BEGIN}string.c is compiling...${MESSAGE_END}}
    ${CC} ${CFLAGS} -c $< -o $@

vga.o: ${SRCDIR}/vga.c ${SRCDIR}/vga.h
    ${info }
    ${info ${MESSAGE_BEGIN}vga.c is compiling...${MESSAGE_END}}
    ${CC} ${CFLAGS} -c $< -o $@

terminal.o: ${SRCDIR}/terminal.c ${SRCDIR}/terminal.h
    ${info }
    ${info ${MESSAGE_BEGIN}terminal.c is compiling...${MESSAGE_END}}
    ${CC} ${CFLAGS} -c $< -o $@

clean: ${BINDIR}/${BINARY}
    rm -rf *.o

makeiso: ${BINDIR}/${BINARY}
    ${info }
    ${info ${MESSAGE_BEGIN}Making an ISO...${MESSAGE_END}}
    cp ${BINDIR}/${BINARY} ${ISODIR}/boot/
    grub-mkrescue ${ISODIR} -o ${RESULTING_ISO}

run: makeiso
    ${info }
    ${info ${MESSAGE_BEGIN}Running the ISO...${MESSAGE_END}}
    qemu-system-x86_64 --cdrom ${RESULTING_ISO}
\$\endgroup\$
  • \$\begingroup\$ Where does boot.s come from? \$\endgroup\$ – Toby Speight Oct 11 at 10:47
  • \$\begingroup\$ @TobySpeight, boot.s is the same as the Bare Bone's boot.s. I did not add it in order to keep the question text as minimal as possible. \$\endgroup\$ – eanmos Oct 11 at 17:44
5
\$\begingroup\$

To complement the Makefile review, here are a few more points that may help you improve your program.

Use VPATH

make already has a number of builtin rules that could be used for this project with no loss of generality. If you omit the colored printing (which I would advocate) and use VPATH, you could use replace most of your Makefile with a single rule:

VPATH = src
${BINDIR}/${BINARY}: boot.o kernel.o string.o vga.o terminal.o
        ${CC} -T linker.ld ${CFLAGS} $^ -o $@ -lgcc

Add missing dependencies

The iso image is dependent on the configuration file as well as the kernel image, so I'd add that file to the Makefile rule.

Specify flags using standard variables

The compiler uses CFLAGS and the assembler uses ASFLAGS by default. Set those variables and simplify your Makefile.

Reorder your targets

Remember that the first target is the default one run if make is invoked with no arguments. For that reason, I'd recommend that the iso image file rule, which is the most derived rule that is a real target, should be first, followed by the target for the kernel, followed by the .PHONY targets.

Create a list of objects

Best practice for a project like this is to use a variable for the created objects like this:

OBJECTS = boot.o kernel.o string.o vga.o terminal.o

This can then be used for both the kernel target and the clean target.

Don't clean files you haven't created

While the existing rule for clean probably works just fine, it will also delete all .o files anywhere in the subtree, whether or not this makefile created them. Better practice is to only delete files created by this makefile.

Revised makefile

Using all of those suggestions, here's what I came up with:

CFLAGS = -m32 -std=c99 -ffreestanding -nostdlib -O0 -Wall -Wextra -pedantic
ASFLAGS = -32

VPATH = src

ISODIR = isodir
KERNEL = ${ISODIR}/boot/kernel.bin
GRUBFCFG = ${ISODIR}/boot/grub/grub.cfg
ISO_IMAGE = kernel.iso
OBJECTS = boot.o kernel.o string.o vga.o terminal.o

.PHONY: clean run

${ISO_IMAGE}: ${KERNEL} ${GRUBCFG}
        grub2-mkrescue ${ISODIR} -o ${ISO_IMAGE}

${KERNEL} : ${OBJECTS}
        ${CC} -T linker.ld ${CFLAGS} $^ -o $@ -lgcc

clean: 
        -rm -f ${OBJECTS} ${KERNEL} ${ISO_IMAGE}

run: ${ISO_IMAGE}
        qemu-system-x86_64 --cdrom ${ISO_IMAGE}

Note that this uses the default compiler and assembler and changes their behavior using flags. It also builds the kernel.bin file where it is needed instead of introducing another step to copy it.

Use something more modern than plain make

I'd recommend looking at autotools or cmake to use as a solid foundation for a maintainable, modern and sophisticated project. For example, neither the original nor revised makefile is capable of out-of-source builds which both of the mentioned tools can easily handle. Further, both support the addition of other things such as unit tests, documentation generation, and multiple types of build (e.g. debug vs. release) that, while possible to do with plain make, quickly becomes more effort than it's worth.

Use the latest standards

The old version of grub uses the original multiboot standard, but things have moved on since then. The current is the multiboot2 standard which is implemented by grub2 among others. I modified your code to use the multiboot2 standard instead in just a few minutes.

Consider unit tests

Unit testing of code provides a higher level of assurence that things are working as you intend. For that reason, I'd highly recommend getting into the habit of introducing unit tests into the code base as early as possible. One way that can be simplified is by using the next suggestion.

Create and use libraries

The string.c file only contains a single function at the moment but is likely to grow as your project does. One efficient way to group and maintain such functions is via the use of libraries, either shared or static. As mentioned above, this also makes unit testing simpler, since one can write tests against a library.

Consider using compiler optimization

The compiler is currently set to do no optimization (-O0). While it may be instructive so see what the unoptimized code looks like, modern compilers are quite good at optimization. It would be a shame to forego all of those benefits.

Consider efficiency and clarity

The code currently includes Terminal_Init which calls Terminal_Clear which calls ClearBackBuffer which calls PutCharToBackBuffer in a loop. We know for a fact that there are no newline characters in that, but the first part of every call to PutCharToBackBuffer checks to see if the character is a newline. I'd suggest that it would make more sense to call PutCharToBackBufferAtCoordinates instead at that location.

Use static where appropriate

Mostly, the use of static is appropriate within this code, but there are some other places it could and should be applied. One example is the Terminal struct within terminal.c.

Don't check build artifacts into version control

Although it's not in the code posted, I notice that the iso and bin files are checked into your git project. Don't do that. Version control is for source files, not the products of them.

\$\endgroup\$
  • \$\begingroup\$ Your answers always are great! Thank you :) \$\endgroup\$ – eanmos Oct 13 at 10:09
6
\$\begingroup\$

Makefile review

We're missing the instruction to tell Make to remove partial results from failing rules; all Makefiles should somewhere contain

.DELETE_ON_ERROR:

makeiso is never up to date. We don't want to re-run it unless the binary has changed, so I'd write:

makeiso: $(RESULTING_ISO)

$(RESULTING_ISO): $(BINDIR)/$(BINARY)
    $(info )
    $(info $(MESSAGE_BEGIN)Making an ISO...$(MESSAGE_END))
    cp $(BINDIR)/$(BINARY) $(ISODIR)/boot/
    grub-mkrescue $(ISODIR) -o $(RESULTING_ISO)

Why does clean depend on having built the binary? I think it should have no dependencies. We could also use the built-in $(RM) rather than writing rm -f.

There's a lot of repetition in the compilation rules:

boot.o: ${SRCDIR}/boot.s
    ${info ${MESSAGE_BEGIN}boot.s is compiling...${MESSAGE_END}}
    ${AS} $< -o $@

kernel.o: ${SRCDIR}/kernel.c
    ${info }
    ${info ${MESSAGE_BEGIN}kernel.c is compiling...${MESSAGE_END}}
    ${CC} ${CFLAGS} -c $< -o $@

string.o: ${SRCDIR}/string.c ${SRCDIR}/string.h
    ${info }
    ${info ${MESSAGE_BEGIN}string.c is compiling...${MESSAGE_END}}
    ${CC} ${CFLAGS} -c $< -o $@

We're duplicating much of what's built-in to make. If we add $(SRCDIR) to the vpath, then we can replace those three rules with just:

string.o: string.h

(Note that there are many more header dependencies not yet mentioned in the rules - but instead of writing them by hand, we should make ourselves a dependency generator here.)

If we really need the fancy printing, then we could override the built-in rules; e.g. for the C sources:

%.o: %.c
        $(info )
        $(info $(MESSAGE_BEGIN)$< is compiling...$(MESSAGE_END))
        $(COMPILE.c) $(OUTPUT_OPTION) $<

The link line can easily be make to work with the built-in rules:

LDFLAGS =+ -T linker.ld
LDLIBS += -lgcc
${BINDIR}/${BINARY}: boot.o kernel.o string.o vga.o terminal.o
    $(LINK.o) $^ $(LDLIBS) -o $@
\$\endgroup\$
  • \$\begingroup\$ Thank you a lot for the answer. Can you please add an example of .DELETE_ON_ERROR usage? \$\endgroup\$ – eanmos Oct 12 at 13:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.