OSDev.org

The Place to Start for Operating System Developers
It is currently Thu Mar 28, 2024 5:37 pm

All times are UTC - 6 hours




Post new topic Reply to topic  [ 9 posts ] 
Author Message
 Post subject: Problems printing to the screen
PostPosted: Sun Feb 24, 2019 8:53 am 
Offline

Joined: Mon Sep 26, 2016 8:25 am
Posts: 18
I have been writing a hobbyist operating system and I am currently implementing a filesystem. The problem I have is not with the filesystem itself, but is with printing text to the screen, but this problem has become very apparent while writing the filesystem.

I have also written a shell, and this is where I have been testing the filesystem implementation. I just finished implementing the `ls` command (which I have tested from inside the kernel and it prints the names of files/folders correctly). This is what it looks like when printing from the kernel (hard coded inside the kernel):
Image

but when I type in `ls` from the shell, I get
Image
As you can see, the text even overflows to the line with the part before the `$`, which is just meant to show `/`, the current working directory.

I am fairly certain that this is a printing (printf?) issue, since this problem with printing is not unique to the `ls` command or even to the shell. The problem with printing arose quite a long time ago with the biggest problem being that `0`'s would not get printed, instead there was just a blank space printed. I chose to ignore the problem rather stupidly, and now the problem has got worse The printing issue became most notable when printing out files and folders, but it has existed for a long time.

So here is my `vprintf.c` and `kprintf.c` files (please ignore any bad coding and lack of comments, since this code was written a long time ago, and I have since learned a lot more about C, good programming practice/readability and more about programming in general):

`kprintf.c`:
Code:
#include "stdio.h"

// Unpacks the variable arguments and passes them to vprintf to be printed to the screen
int kprintf(const char* fmt, ...)
{
    va_list args;
    va_start(args, fmt);

    // Pass the arguments to vprintf to be unpacked and printed and then get the number
    // of characters written to the screen as returned by vprintf
    int num_chars_written = vprintf(fmt, args);

    va_end(args);

    return num_chars_written;
}


`vprintf.c`:
Code:
#include <stdarg.h>

#include "stdio.h"
#include "../size_t.h"

extern size_t strlen(const char *str);
extern char *strrev(char *str);

int vprintf(const char *fmt, va_list args)
{
   unsigned int j;
   unsigned int num_chars_written = 0;

   for (j = 0; j < strlen(fmt); j++)
   {
      /* check for format specifier */

      if (fmt[j] == '%')
      {
         /* print a number to the screen */

         if (fmt[j + 1] == 'd')
         {
            int i = va_arg(args, int);
            char *str = "";

            // if (strncmp("0x", (char *) i, 2) == 0) { //convert to decimal }

            if (i == 0 || i == '0')
            {
               puts("0");
            }
            else
            {
               // Convert the integer to a string, increase num_chars_written and print the integer
               itoa(i, str, 10);
               num_chars_written += strlen(str);

               puts(str);
            }
         }

         /* prints a character to the screen */

         else if (fmt[j + 1] == 'c')
         {
            int c = va_arg(args, int);

            num_chars_written++;
            putchar(c);
         }

         /* prints a string to the screen */

         else if (fmt[j + 1] == 's')
         {
            char* s = va_arg(args, char*);
            num_chars_written += strlen(s);

            puts(s);
         }

         /* check if number is to be converted to hex */

         else if (fmt[j + 1] == 'x' || fmt[j + 1] == 'X')
         {

            int j = 0;

            int i = va_arg(args, int);
            char *str = "";
            itoa(i, str, 10);
            // Set the maximum number of characters to the length of the number as a string, leaving a space for the null byte
            char hex[strlen(str)];

            // Print hex characters in lowercase if lowercase x was used, otherwise print hex in uppercase
            if (fmt[j + 1] == 'x')
            {
               char hex_chars_lower[] = "0123456789abcdef";

               do
               {
                  hex[j++] = hex_chars_lower[i % 16];
               } while ((i /= 16) > 0);
            }
            else
            {
               char hex_chars_upper[] = "0123456789abcdef";

               do
               {
                  hex[j++] = hex_chars_upper[i % 16];
               } while ((i /= 16) > 0);
            }

            hex[j] = '\0';
            strrev(hex);

            hex[j + 1] = '\0';

            num_chars_written += strlen(hex);

            puts(hex);
         }

         /* check if pointer is to be printed */

         else if (fmt[j + 1] == 'p')
         {
            void *ptr = va_arg(args, void *);

            kprintf("%d", ptr);
         }

         /* prints a percent (%) */

         else if (fmt[j + 1] == '%')
         {
            num_chars_written++;
            putchar('%');
         }

         /* prints a new line (cursor goes to the beginning of the next line) */

         else if (fmt[j] == '\n')
         {
            // Calls to kprintf() with '\n' result in "\r\n" being printed
            num_chars_written += 1;

            putchar('\n');
         }

         /* prints a tab character */

         else if (fmt[j] == '\t')
         {
            num_chars_written++;

            putchar('\t');
         }
      }

      /* else, print the character */

      else if (fmt[j - 1] != '%')
      {
         num_chars_written++;

         putchar(fmt[j]);
      }
   }

   return num_chars_written;
}



For reference, `putchar` just calls `terminal_putc` which is as follows (taken from osdev bare bones tutorial):
Code:
// Write a character to the screen
void terminal_putchar(char c)
{
   char *vidmem = (char *)0xB8000;

   // '\n' sends cursor to beginning of next line
   if (c == '\n')
   {
      x = 0;
      y += 2;
   }
   else
   {
      unsigned int i = y * VGA_WIDTH + x;

      vidmem[i] = c;
      move_cursor(x + 1, y);
      i++;
      vidmem[i] = terminal_get_color();
      move_cursor(x + 1, y);
   }

   if (y > 48)
   {
      terminal_scroll();

      y -= 2;
   }
}


and `puts`, similarly, just calls `terminal_write` (again, taken from osdev bare bones tutorial):
Code:
// Write a string to the screen
void terminal_write(char *str)
{
   for (unsigned int i = 0; i < strlen(str); i++)
   {
      terminal_putchar(str[i]);
   }
}


And here is the function that is executed when typing in `ls`:
Code:
// List the contents of the current directory
void xsh_ls(char **args)
{
   char *dirname = args[1];

   DIR *dir = opendir(dirname);

   if (dir == NULL)
   {
      kprintf("ls: cannot access '%s': no such directory", dirname);
      return;
   }

   // Print out each directory name in `sub_dirnames`
   for (unsigned int i = 0; i < dir->total_entries; i++)
   {
      if (i != dir->total_entries - 1)
         puts(sub_dirnames[i]);
      // kprintf("%s\n", sub_dirnames[i]);
      else
         puts(sub_dirnames[i]);
      // kprintf("%s", sub_dirnames[i]);
   }

   kprintf("\n\n\n");

   closedir(dir);
}


Any help would be greatly appreciated! If any extra information is needed, please let me know and I will provide it. Thanks!


Top
 Profile  
 
 Post subject: Re: Problems printing to the screen
PostPosted: Mon Feb 25, 2019 12:21 am 
Offline
Member
Member

Joined: Fri Aug 26, 2016 1:41 pm
Posts: 671
I noticed you do this kind of thing in more than one spot:
Code:
            char *str = "";
            itoa(i, str, 10);
char *str = '" allocates 1 byte of memory that contains the NUL terminator. So it is a buffer that can hold a single character. Your itoa is going to write more than 1 byte of data into the string. That will cause corruption. Maybe you wish to do something like char str[33]; That is room for a 32-bit value to be represented in base 2 (binary) + NUL terminating character. Base 2 is the worst case scenario. You make this error in more than one place so you should look to fix all occurrences of this issue.


Top
 Profile  
 
 Post subject: Re: Problems printing to the screen
PostPosted: Mon Feb 25, 2019 1:29 am 
Offline
Member
Member

Joined: Tue Mar 04, 2014 5:27 am
Posts: 1108
MichaelPetch wrote:
I noticed you do this kind of thing in more than one spot:
Code:
            char *str = "";
            itoa(i, str, 10);
char *str = '" allocates 1 byte of memory that contains the NUL terminator. So it is a buffer that can hold a single character. Your itoa is going to write more than 1 byte of data into the string. That will cause corruption. Maybe you wish to do something like char str[33]; That is room for a 32-bit value to be represented in base 2 (binary) + NUL terminating character. Base 2 is the worst case scenario. You make this error in more than one place so you should look to fix all occurrences of this issue.


It's illegal to write into a string literal even if it's long enough.


Top
 Profile  
 
 Post subject: Re: Problems printing to the screen
PostPosted: Mon Feb 25, 2019 4:20 am 
Offline
Member
Member

Joined: Fri Aug 26, 2016 1:41 pm
Posts: 671
Agreed given it is undefined behaviour. I just didn't take the time at 2am to explain why char *str=""; and char str[]=""; were different.Also a lesson in there about using `const` to capture this type of problem (or at least try to use -Wwrite-strings GCC option.


Top
 Profile  
 
 Post subject: Re: Problems printing to the screen
PostPosted: Mon Feb 25, 2019 9:17 am 
Offline
Member
Member

Joined: Mon Mar 25, 2013 7:01 pm
Posts: 5100
0xBADC0DE wrote:
(please ignore any bad coding and lack of comments, since this code was written a long time ago, and I have since learned a lot more about C, good programming practice/readability and more about programming in general)

Why not refactor the code with your new knowledge? You might spot some of the issues along the way.

Code:
#include "../size_t.h"

Delete size_t.h and include stddef.h instead.

Code:
if (i == 0 || i == '0')

Why do you want printf("%d", 48) to print 0?

Code:
int j = 0;

In your code to display hexadecimal values, you declare a second variable named "j" and then try to refer to both definitions of "j". (And why not call itoa with a base of 16? Of course you must also fix the declaration of the temporary string to store the result, as mentioned above.)

Code:
else if (fmt[j] == '\n')
...
else if (fmt[j] == '\t')

You don't need special case handling for these characters in vsprintf.

Code:
else if (fmt[j - 1] != '%')

This is not the correct way to skip the format specifier. Instead, you should increment "j" when you reach the %. (This also allows you to change every use of "fmt[j+1]" into "fmt[j]".) When you want to extend your vprintf to support longer format specifiers, you can increment "j" to consume each of the additional characters.

Code:
char *vidmem = (char *)0xB8000;
...
unsigned int i = y * VGA_WIDTH + x;

You declare the pointer to video memory as char*, so each character on the screen takes up two units. The correct formula for indexing a character based on its coordinates is "(y * VGA_WIDTH + x) * 2". You'll also need to correct your newline handling and scrolling to take this into account.


Top
 Profile  
 
 Post subject: Re: Problems printing to the screen
PostPosted: Mon Feb 25, 2019 9:58 am 
Offline
Member
Member
User avatar

Joined: Fri Oct 27, 2006 9:42 am
Posts: 1925
Location: Athens, GA, USA
Octocontrabass wrote:
Code:
char *vidmem = (char *)0xB8000;
...
unsigned int i = y * VGA_WIDTH + x;

You declare the pointer to video memory as char*, so each character on the screen takes up two units. The correct formula for indexing a character based on its coordinates is "(y * VGA_WIDTH + x) * 2". You'll also need to correct your newline handling and scrolling to take this into account.


@0xBADC0DE: as an alternative approach, I would recommend to define a struct type for the text video character cells, and define a number of ancillary variables as well, like so:

Code:
uint16_t MAX_COL = 80;     /* set this as appropriate for the text mode */
uint16_t MAX_ROW = 60;     /* ditto* /


/* IIRC, the attribute byte comes first, followed by the actual character. If I am wrong about the order of the two bytes, please correct me. */
typedef char_cell {
    uint8_t attrib;
    char glyph;
} CHAR_CELL;


/* You can actually define up to four text pages, which makes it easy to page-flip if you need to.
   However, you would need to tell the system which hardware page pointer to use, I think.
   If anyone can elaborate on this, please do. */
CHAR_CELL* video_page[] = {
    (CHAR_CELL *) 0xB8000,
    ...                                          /* the number and location of the other pages will depend on the video mode */ 
};

CHAR_CELL *vidmem = page[0];  /* 'vidmem' is the handle to the start of the current text buffer */
uint16_t row = 0, col = 0;                /* offsets for the current cursor position */


This has the added advantage of cleanly separating the attribute bytes from the character bytes. You do need to access the characters with a field pointer, however:

Code:
vidmem[col + (row * MAX_COL)] ->glyph = 'a';


Note that you would not insert the newline or tab characters into the text buffer; rather, for newline, you would advance the cursor to the start of the next line of the buffer

Code:
col = 0;
row++;
if (row > MAX_ROW) { ... }      /* handle scrolling or other appropriate operations here */


Similarly, for a tab character, you would advance col according to the tab stops, presumably clearing existing characters when appropriate (depending on what the program needs to do).

Also, don't forget to set the Text Mode Cursor as you advance the text.

_________________
Rev. First Speaker Schol-R-LEA;2 LCF ELF JAM POEE KoR KCO PPWMTF
Ordo OS Project
Lisp programmers tend to seem very odd to outsiders, just like anyone else who has had a religious experience they can't quite explain to others.


Top
 Profile  
 
 Post subject: Re: Problems printing to the screen
PostPosted: Thu Feb 28, 2019 11:48 am 
Offline

Joined: Mon Sep 26, 2016 8:25 am
Posts: 18
@MichaelPetch @alexfru thanks for pointing that out. I will fix that problem, thanks.

@Octocontrabass yes, I will do that. I will definitely need to rewrite my kprintf function.
And the reason I created and included "size_t.h" , and not "stddef.h", is that I wanted to write those files myself, and make my operating system as unreliant on other, external code as possible. Would it still be better to include "stddef.h", or should I write my own "stddef.h" header file?
And I did "if (i == 0 || i == '0')", because, as I mentioned in my post, when I try to print a "0" using the "%d" format specifier, it just prints out nothing. I was trying to get it to print a "0", which never worked, so maybe something wrong with my "puts()" or "terminal_putchar()" functions.
Code:
i = (y * VGA_WIDTH + x) * 2
does make sense and I'll change it, but why does it work the way I had it? It's also how I've seen it in other tutorials as well.

@Schol-R-LEA I'll definiteily take a look at doing my text output that way, thanks! What is the idea behind having the "video_page" array? Would that mean you can 4 screens of text and switch between them, kind of like how linux has tty's? And yes, you are right, the attribute byte does come first, and then the color.
And would a text mode cursor be required, since I already have my own "cursor", which is just a gray background filled block?


Top
 Profile  
 
 Post subject: Re: Problems printing to the screen
PostPosted: Thu Feb 28, 2019 12:38 pm 
Offline
Member
Member

Joined: Mon Mar 25, 2013 7:01 pm
Posts: 5100
0xBADC0DE wrote:
And the reason I created and included "size_t.h" , and not "stddef.h", is that I wanted to write those files myself, and make my operating system as unreliant on other, external code as possible. Would it still be better to include "stddef.h", or should I write my own "stddef.h" header file?

You should use the stddef.h provided by the compiler.

0xBADC0DE wrote:
And I did "if (i == 0 || i == '0')", because, as I mentioned in my post, when I try to print a "0" using the "%d" format specifier, it just prints out nothing. I was trying to get it to print a "0", which never worked, so maybe something wrong with my "puts()" or "terminal_putchar()" functions.

You're also using itoa() to convert the number to a string. Have you checked to make sure itoa() works properly with 0?

0xBADC0DE wrote:
Code:
i = (y * VGA_WIDTH + x) * 2
does make sense and I'll change it, but why does it work the way I had it? It's also how I've seen it in other tutorials as well.

Other tutorials probably follow Schol-R-LEA's advice and define the frame buffer in 16-bit units, so each character displayed on the screen is 1 unit in the array (instead of two 8-bit units per character).


Top
 Profile  
 
 Post subject: Re: Problems printing to the screen
PostPosted: Sat Mar 02, 2019 4:02 am 
Offline

Joined: Mon Sep 26, 2016 8:25 am
Posts: 18
I managed to fix the problem I was having with it printing. It wasn't related so much to printing as it was to how the entry names were being stored in an array.

I have the following code inside the filesystem driver to open a directory:

Code:
DIR *opendir(char *dirname)
{
    DIR *dir = kmalloc(sizeof(DIR));
    dir->entries = kmalloc(sizeof(struct dirent) * (total_rootfs_dirs + total_rootfs_entries));

    // Get a list of every directory with its name in the filesystem
    struct entry entries[total_rootfs_entries];
    char **entry_names = kmalloc(total_rootfs_dirs + total_rootfs_files);
    get_entry_names(entry_names, entries, DIRECTORY_ENTRY | FILE_ENTRY);

    // Replace '.' with the current directory
    if (strlen(dirname) > 0 && dirname[0] == '.')
        strcpy(dirname, current_dir);

    // If `dirname` was the root directory (ie. a '/'), then list all files in the filesystem (depth=1)
    if (strcmp(dirname, PATH_SEPARATOR) == 0)
    {
        // Holds the entries within the opened directory
        unsigned int i = 0;

        // Get a list of the directory's entries
        for (i = 0; i < total_rootfs_dirs + total_rootfs_files; i++)
        {
            struct dirent dirent;

            strcpy((char *)dirent.name, entry_names[i]);
            memcpy(&dir->entries[i], &dirent, sizeof(dirent));
            // kprintf("%s  ", dir->entries[i].name); // **THIS LINE PRINTS ALL ENTRY NAMES CORRECTLY**
        }

        dir->fd = current_file_descriptor++;

        // Allocate memory for the directory name and store the directory name in the struct
        dir->total_entries = i;

        kfree(entry_names);

        return dir;
/* ...snip... */


My "DIR" struct is like this:
Code:
typedef struct _DIR
{
    uint16_t fd;
    int8_t *dirname;
    uint32_t current_dir;
    uint32_t total_entries;
    uint8_t *offset;
    struct dirent *entries;
} DIR;


"total_entries" just holds a list of "dirent" structs, which just holds the name of the entries in the filesystem:
Code:
struct dirent
{
    uint8_t *name;
};


Basically, the above code just checks if the directory opened was the root directory ("/") and just gets a list of each entry name in the filesystem. It then creates a "dirent" struct, stores the name in there, and stores that struct inside the "entries" member of the struct, so that the entry name can be read from that later.

Then I have this inside "readdir()":
Code:
struct dirent *readdir(DIR *dir)
{
    if (dir->current_dir < dir->total_entries)
    {
        struct dirent dirent = dir->entries[dir->current_dir];

        kprintf("%s  ", dir->entries[dir->current_dir].name); // **NOT PRINTING CORRECTLY**

        dir->current_dir++;

        // return dirent;
    }
    else
    {
        return NULL;
    }
}


I know I'm not returning anything thing from the above code, but it's just for testing, because right now I'm just printing things out, and the above code outputs
Code:
/bin/ls /bin/ls /bin/ls
as many times as there are entries in the filesystem, but it's not related to the printing problem I described earlier.

Now the problem is that the "kprintf" inside the "opendir()" function is printing out the entry names correctly, but the "kprintf" inside the "readdir()" function is not printing correctly. I just can't understand how it's being printed out correctly (and added to the array/list of entries inside the "DIR" struct), but I try printing using the exact same code and the exact same "DIR" struct inside "readdir()" and it won't print correctly.

Inside "kernel.c" I have:
Code:
DIR *dir = opendir("/");
struct dirent *dirent;

while ((dirent = readdir(dir)) != NULL)
{
   // kprintf("%s  ", dirent->name);
}

for testing purposes. I commented out the "kprintf", because I am just testing the printing inside the "opendir()" and "readdir()" functions for now

Something is going wrong somewhere being these two functions, and I can't quite figure out what


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 9 posts ] 

All times are UTC - 6 hours


Who is online

Users browsing this forum: DotBot [Bot] and 71 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
Powered by phpBB © 2000, 2002, 2005, 2007 phpBB Group