Multicore support, new standard library, etc... #287
Conversation
Fix infinite recursion due to atomic set/add for size_t referring to itself
… ensuring return address in top stack frame is 0
…classes + refactoring
|
There is way too much in this pull request. We need to split this up into separate parts. Some is whitespace-reformatting that I would definitely not merge. Also some other parts I might not merge --> we should split this up as much as possible so that we can decide piece by piece whether or not to merge it. |
|
I also would like to omit the EASTL tests, no need to ship those to everyone. |
|
The EASTL/test/packages folder actually contains a sub-library (EABase) that the EASTL depends on. (No idea why it's in that location but it's there) |
|
The EASTL is imported as a git subtree to allow updates from upstream. I tried to modify it as little as possible so merging of updates is easier. |
|
That totally makes sense. From my perspective merging updates has a
lower priority than getting pitfalls out of the way. So if we can
increase convenience for the students at the cost of a bit of
inconvenience for the one merging the update, I'd take it.
|
|
A subtree is more or less the same as if the files were added normally in the repo, so they can be modified as needed without even having to know it's a subtree. The only difference is that it records the external commit where the files came from to to allow for a clean merge of updates using git tools and history instead of having to manually copy over files and resolving conflicts by hand. |
|
The one external dependency of EASTL (the aforementioned EABase, which more or less just contains some basic libc level functions) was included as a submodule in the original source. Since git submodules can be a nightmare and would absolutely cause trouble (recursive checkout required, etc...) I also directly included that as a subtree. |
|
that makes a lot of sense. i guess we should figure out how to proceed here. is there any chance that we can split this up? maybe also a meeting would make sense to go through the code and see how we can merge which part. |
arch/x86/common/source/assert.cpp
Outdated
| @@ -81,7 +81,7 @@ __attribute__((noreturn)) void pre_new_sweb_assert(const char* condition, uint32 | |||
| if (ArchMulticore::numRunningCPUs() > 1) | |||
| { | |||
| ArchMulticore::stopAllCpus(); | |||
| volatile size_t wait = 0x10000000; | |||
| eastl::atomic<size_t> wait = 0x10000000; | |||
There was a problem hiding this comment.
I'm not sure this is correct. indeed volatile is deprecated for many use cases where users commonly had the misconception that volatile would provide some form of thread safety... but here we literally want what volatile is there for: "do not optimize out any operations on this variable".
do i misremember that atomic<> may be optimized out?
There was a problem hiding this comment.
That line no longer exists in the current version of the file.
https://github.com/Woazboat/sweb/blob/multicore/arch/x86/common/source/assert.cpp#L98
The reason why it was there in the first place was as a quick interim measure to prevent backtraces and assert messages from individual cpu cores from overlapping and interspersing, but that has shortly afterwards been replaced by proper locking/serialization.
(the version with volatile also no longer compiled with the volatile deprecation changes)
|
A big issue when attempting to split this up is that it includes a lot of general baseline improvements and utility functionality that is then used in a lot of other places, as well as changes that require (minor) adaptation throughout the codebase to work properly. Individual components should be encapsulated as best as they can in the code, but the changes how they came to be that way were not necessarily always close together in time and have gone trough many revisions. I think a meeting where we can go over the changes would be best Merging this into a branch until it can be integrated into main would allow others/students to try it out without having to go through the process of getting it from an external repo (but yes, it wouldn't change the end result) |
|
make debug seems to be broken (at least on ubuntu 22.04). |
|
Yes, I'm afraid we're running into a(nother) bug in minixfs there. With debug infos enabled and the new dwarf debug infos now that stabs is depecated, the kernel binary is large enough to trigger it. I changed the build system to always make the unstripped kernel binary available as EDIT: backtrace was a different issue that is now fixed |
|
The internal backtrace is a very important debug feature for students. |
I have actually started to write an ext2 implementation for exactly that reason. It's just as simple as minixfs but a lot more practical. The code will hopefully also be a lot easier to understand than the current rather messy minixfs implementation. Maybe that will prompt students to do more file system related things in the future if they can actually understand what's going on there. I added some assertions so that an overflow/out of bounds write is at least detected |
|
The internal backtraces should be fixed now. I think we can simply remove the make debug target as it doesn't really serve a purpose. The binary with debug information is always available outside qemu via the saved unstripped file while the stripped version with the custom debug info is loaded inside qemu. The only thing |
|
I just tested this branch and it looks quite nice :) However, The fix is quite simple though, just don't copy data when the string is empty (since accessing commit c4b9d29bcfa8e0a48ea220c6430a2906b8b883be (multicore)
Author: Ferdinand Bachmann <[email protected]>
Date: Sat Mar 25 13:58:44 2023 +0100
dwarf++: fix UB when getting a string of zero length
diff --git a/utils/add-debug/dwarf/cursor.cc b/utils/add-debug/dwarf/cursor.cc
index 19902da8..1eb6c49b 100644
--- a/utils/add-debug/dwarf/cursor.cc
+++ b/utils/add-debug/dwarf/cursor.cc
@@ -86,7 +86,7 @@ cursor::string(std::string &out)
size_t size;
const char *p = this->cstr(&size);
out.resize(size);
- memmove(&out.front(), p, size);
+ if (size) memmove(&out.front(), p, size);
}
const char *PS: Works fine with GCC 13 |
|
Thanks for tracking that down! Under what circumstances did the compiler produce an empty dwarf string? The libelfin library used for ELF/DWARF parsing unfortunately seems to have a few issues. It actually incorrectly parsed 32 bit ELF files, so debug info on 32 bit platforms didn't work at all previously. Woazboat@48433ac That particular bug was already fixed in a new version of the library that I have since updated (Woazboat@b653cc4), but apparently there are unfortunately still bugs lingering in there and it seems to be no longer maintained. Open CVEs for the libelfin library Edit: Turns out there's even an open pull request for exactly the bug you ran into... aclements/libelfin#63 |
… string Bug found in SWEB by https://github.com/Ferdi265 Open pull request on the upstream project that's been ignored so far... aclements/libelfin#63
Fix compilation on GCC 13 IAIK#292
Changes... pretty much everything. Lots of new features and a ton of bug fixes.
From a student point of view, things shouldn't actually be too different initially as the changes are intentionally mostly in the background without affecting the existing interfaces too much
Probably better to merge this into a staging branch first and then into main later (github pull requests do not allow you to create new branches however)
Changes
SMP/multicore
Scheduler
Interrupts & Timers
Drivers
Libraries + utility code
Kernel Initialization
Feature detection
File system
Debugging
Debug output
Debug info
Other debug information
Userspace
Build system
Misc. other changes
Misc. Bugfixes