Make runtime call hardware init functions automatically #195

Hazematman · 2021-10-10T19:59:26Z

Modify the c runtime to automatically call initialize functions
for interrupts and for cop1.

Addtionally add cop1 init routine flash denormalized floats to 0.

As a note this pull request also modifies the testrom to mark test_dma_read_misalign as TEST_FLAGS_NO_BENCHMARK. This was done as when running on hardware I found that this test would randomly fall because of the execution time.

Modify the c runtime to automatically call initalize functions for interrupts and for cop1. Addtionally add cop1 init routine flash denormalized floats to 0.

rasky

Great PR! We're doing a great step here. We finally have constructors working and we simplify the libdragon initialization for beginners.

rasky · 2021-10-10T21:13:50Z

n64.ld

@@ -20,6 +20,8 @@
 OUTPUT_FORMAT ("elf32-bigmips", "elf32-bigmips", "elf32-littlemips")
 OUTPUT_ARCH (mips)
 EXTERN (_start)
+EXTERN(__init_interrupts)
+EXTERN(__init_cop1)


I think the idea here is that the compiler will auto-remove constructors of modules that are not used by the application. This is actually a good feature for us, since we want in general to reduce code size.

In this specific case, I would say that the interrupt constructor doesn't need extern: if I manage to do a main that never calls disable/enable interrupts (!), then I would not need init_interrupts as well. The interrupts would stay disabled and that's it.

On the other, I think the issue is that you put the constructor in cop1.c which is a new object file and otherwise unreferenced. If you moved that to something like n64sys.c, there wouldn't be a need for this.

What do you think?

I like the idea of not having a new file for this one routine. I'll move it into n64sys.c, I think the EXTERN will still be required to make sure the cop1 bits are set, as I don't think every program is guaranteed to use a symbol from n64sys.c

I think n64sys is always included because it contains get_memory_size which is used by sbrk. But I see the point of this being a little fragile in general, so the EXTERN is probably worth it anyway.

rasky · 2021-10-10T21:15:02Z

src/cop1.c

+    uint32_t fcr31 = C1_FCR31();
+
+    /* Set FS bit to allow flashing of denormalized floats */
+    fcr31 |= C1_FS_BIT;


I think you should elaborate a little bit on this. I personally don't know what means "flashing a denomal", so I'd explain here (or a in a function-level comment) why this is required and how it is good.

rasky · 2021-10-10T21:16:05Z

tests/test_denormalized.c

+    volatile float x = 1.0f;
+    x /= FLT_MAX;
+
+    ASSERT(x == 0.0f, "Denormalized float was not flushed to zero");


Also a comment here to explain that we're testing that an exception is not generated when a denormal is created.

(Technically, a test that could fail with an exception should trap it to avoid trashing the whole testsuite when it fails, but that's something we don't have helpers for, so don't worry. I'm sure it would be a new rabbit hole...)

rasky · 2021-10-10T21:17:26Z

tests/test_denormalized.c

@@ -0,0 +1,11 @@
+#include <float.h>
+
+void __initalize_cop1();


rasky · 2021-10-10T21:18:16Z

tests/test_denormalized.c

@@ -0,0 +1,11 @@
+#include <float.h>


Maybe I'd call the file test_cop1 or test_float. Typically a file might contain multiple related tests, so the name should be more generic.

rasky · 2021-10-10T21:18:25Z

tests/testrom.c

@@ -202,12 +203,11 @@ static const struct Testsuite
 	TEST_FUNC(test_eepromfs,                   0, TEST_FLAGS_IO),
 	TEST_FUNC(test_cache_invalidate,    	1763, TEST_FLAGS_NONE),
 	TEST_FUNC(test_debug_sdfs,             	   0, TEST_FLAGS_NO_BENCHMARK),
-	TEST_FUNC(test_dma_read_misalign,       7003, TEST_FLAGS_NONE),
+	TEST_FUNC(test_dma_read_misalign,       7003, TEST_FLAGS_NO_BENCHMARK),
+    TEST_FUNC(test_denormalized,               0, TEST_FLAGS_NO_EMULATOR),


Tab vs space here

rasky · 2021-10-10T21:19:08Z

tests/testrom.c

@@ -202,12 +203,11 @@ static const struct Testsuite
 	TEST_FUNC(test_eepromfs,                   0, TEST_FLAGS_IO),
 	TEST_FUNC(test_cache_invalidate,    	1763, TEST_FLAGS_NONE),
 	TEST_FUNC(test_debug_sdfs,             	   0, TEST_FLAGS_NO_BENCHMARK),
-	TEST_FUNC(test_dma_read_misalign,       7003, TEST_FLAGS_NONE),
+	TEST_FUNC(test_dma_read_misalign,       7003, TEST_FLAGS_NO_BENCHMARK),


Can you please leave this with benchmark? It doesn't fail on many consoles so there must be something else that we should look into. If you change this, we will simply forget about it.

rasky · 2021-10-10T21:20:52Z

src/interrupt.c

+void init_interrupts()
+{
+    __init_interrupts();
+}


__init_interrupts is idempotent, but since it's been already called by the constructor there's really no need to call it again, I would say. In fact, you could even change this to function to a static inline function in the header file that is empty and thus removed even if compiled.

Because of -Werror, if we define a function and then never use it an error will generated. Perhaps we just make it an macro instead that evaluates to nothing?

I don't see this problem in rsp.h for deprecated functions, but anyway you can use __attribute__((used)) to disable that warning.

I think I was missing the inline part, that seems to have fixed it. I pushed a new commit that should address all of your comments.

Additionally clean up cop1 testrom code

rasky

Thanks! This is good to go IMO

rasky · 2021-10-11T07:47:45Z

include/cop1.h

@@ -34,6 +34,8 @@
 #define C1_CAUSE_INVALID_OP         0x00010000
 #define C1_CAUSE_NOT_IMPLEMENTED    0x00020000

+#define C1_FS_BIT                   (1<<24)


I think we should name this C1_FCR31_FS. I think we usually put the register name first for which the mask must be used.

rasky · 2021-10-11T07:49:25Z

src/n64sys.c

+       is generated by any floating point operations. In order to prevent
+       this exception we set the FS bit in the fcr31 control register to
+       instead "flash" and "flush" the denormalized number. To understand 
+       the flashing rules please read pg. 213 of the VR4300 programmers manaul */


Typo "manual"

sp1187 · 2021-10-11T08:01:48Z

src/n64sys.c

@@ -251,4 +251,27 @@ void wait_ms( unsigned long wait_ms )
    wait_ticks(TICKS_FROM_MS(wait_ms));
 }

+
+/**
+ * @brief Initalize COP1 with default settings that prevent undesirable exceptions.


Typo "Initialize"

Hazematman · 2021-10-11T13:57:24Z

Thanks for all the feedback everyone :) I've pushed a new commit that resolves the above mentioned issues

sp1187 · 2021-10-11T14:25:12Z

src/n64sys.c

+ */
+__attribute__((constructor)) void __init_cop1()
+{
+    /* Read initalized value from cop1 control register */


Typo "initialized".

Thanks! It seems that although english is my first language my proof reading skills are very bad 😅 I've pushed a new commit with that typo fixed

@sp1187 could you review the final patch, and sign off if you think its good?

In DragonMinded#195, we fixed a bug (off-by-one) in __do_global_ctors that caused the last constructor not to be called in C programs (using ld linker). This in turn broke C++ software, that uses g++ as linker. We found out that ld and g++ creates different constructor tables. We attempted a first fix in DragonMinded#197 by switching n64.mk to use g++, but this broke the usage of ld linker; so we fixed it again in DragonMinded#201 by separating the codebase for ld (__do_global_ctors) vs g++ (__wrap___do_global_ctors). Unfortunately, in doing so in DragonMinded#201, we reverted to the version of __do_global_ctors that had the initial bug, effectively reverting DragonMinded#195. So, all in all, we still have code built by ld broken because the last constructor (eg: init_interrupts) is not called. So this PR introduces again the fix in DragonMinded#195, hopefully fixing it finally for good.

In #195, we fixed a bug (off-by-one) in __do_global_ctors that caused the last constructor not to be called in C programs (using ld linker). This in turn broke C++ software, that uses g++ as linker. We found out that ld and g++ creates different constructor tables. We attempted a first fix in #197 by switching n64.mk to use g++, but this broke the usage of ld linker; so we fixed it again in #201 by separating the codebase for ld (__do_global_ctors) vs g++ (__wrap___do_global_ctors). Unfortunately, in doing so in #201, we reverted to the version of __do_global_ctors that had the initial bug, effectively reverting #195. So, all in all, we still have code built by ld broken because the last constructor (eg: init_interrupts) is not called. So this PR introduces again the fix in #195, hopefully fixing it finally for good.

Make runtime call hardware init functions automatically

543186d

Modify the c runtime to automatically call initalize functions for interrupts and for cop1. Addtionally add cop1 init routine flash denormalized floats to 0.

rasky reviewed Oct 10, 2021

View reviewed changes

Hazematman added 2 commits October 10, 2021 18:43

Clean up init_interrupts and init_cop1 code

2f1abe7

Additionally clean up cop1 testrom code

Update init_interrupts deprecated logic to be correct

b0e625c

rasky approved these changes Oct 11, 2021

View reviewed changes

sp1187 reviewed Oct 11, 2021

View reviewed changes

Resolve typos

5f9c1c4

sp1187 reviewed Oct 11, 2021

View reviewed changes

Fix typos in comments

1504fd7

sp1187 approved these changes Oct 13, 2021

View reviewed changes

anacierdem merged commit 2bd68cf into DragonMinded:trunk Oct 13, 2021

anacierdem mentioned this pull request Oct 21, 2021

Fix breaking change #201

Merged

rasky mentioned this pull request Nov 2, 2021

do_ctors: fix (again) constructors when using ld linker #222

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make runtime call hardware init functions automatically #195

Make runtime call hardware init functions automatically #195

Hazematman commented Oct 10, 2021

rasky left a comment

rasky Oct 10, 2021

Hazematman Oct 10, 2021

rasky Oct 10, 2021

rasky Oct 10, 2021

rasky Oct 10, 2021

rasky Oct 10, 2021

rasky Oct 10, 2021

rasky Oct 10, 2021

rasky Oct 10, 2021

rasky Oct 10, 2021

Hazematman Oct 10, 2021

rasky Oct 10, 2021

Hazematman Oct 10, 2021

rasky left a comment

rasky Oct 11, 2021

rasky Oct 11, 2021

sp1187 Oct 11, 2021

Hazematman commented Oct 11, 2021

sp1187 Oct 11, 2021

Hazematman Oct 11, 2021

Hazematman Oct 13, 2021

Make runtime call hardware init functions automatically #195

Make runtime call hardware init functions automatically #195

Conversation

Hazematman commented Oct 10, 2021

rasky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rasky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hazematman commented Oct 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment