-
Notifications
You must be signed in to change notification settings - Fork 81
CLI: bugfix and improvements #166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Before this commit, it could occur that QCoreApplication::exit() is called before the call to before QCoreApplication::exec(). As a result, the call to QCoreApplication::exit() gets ignored and QtRVSim will never terminate. A simple way to reproduce the bug is to call QtRVSim on an empty assembly file: `qtrvsim_cli --asm empty.S`. This commit fixes the issue by moving initialization code (including the argument parsing and assembler invocation) into the Qt main loop. For that, code from main.cpp is moved into a new class CLIMain. Some previously local variables of the main function are now members of CLIMain.
This feature can be useful when a user only wants to see critical messages.
Hello @mgschossmann, thanks for contribution. I agree with added functionality and options. I agree that current errors reporting and exits through old C exit() functions is against C++ and Qt principles. On the other hand, for me in a CLI case paring should be done before Qt exec. But I agree that it doe snot play well with Qt concept. I will discuss that with my colleagues, i.e. @fvacek who works with Qt from 2.0 days when he even bought the library for his application done for hospitals and doctors. I am not sure if some change or extension in the reporting can cause problems with our student works evaluation in Makefile(s) in b35apo/stud-support:seminaries/qtrvsim and Python based evaluation for web https://gitlab.fel.cvut.cz/b35apo/qtrvsim-eval-web. But that can be corrected and we should switch So in general, I would be happy if we move forward but it can be stuck on our side. Please, remind us again in such case in some week or month. As for the GDB server, it is great feature which I have on my long term list of wishes. But I would be strict there even for our users confidence that it creates a socket only when explicitly defined on command line for CLI and for GUI probably only when enabled after startup and if GUI application is finished and started again then reguire again to check the option on. It could be a GUI command line option as well. In general, some GUI application update to accept some command-line options could be uses-full in longer run. It would be even great if WASM version ca be configured through URL parameters to match setting of given task evaluator when linked from web eval task. @fvacek has implemented such arguments passing in silicon-heaven/shvspy. GDB server for 64-bits too makes sense for me because RV64 is standard for Linux and we can compile applications (with newlib) which runs on x86 Linux under QEMU user and really natively on RV targets. But 32-bit is nice option for start and for basic teaching it is enough/simpler/probably better. As for our plans, there is ongoing thesis project of MMU support #165 which should be submitted for deference in May 2026. Some input, ideas are welcomed. Something is already working but I expect prototype in state to test and do a review by the end of the year. There is another pull request pending (too long) #138 . It is for me and @jdupak review to check the impact. I have hope that we will find a way to adapt code to some generic class which can be used with line numbers from internal editor and assembler but would be prepared to add even some simple/small/minimal DWARF to code location mapping for ELF files. But small and DWARF seems to be an oxymoron. I have found some reasonably sized library for native DWARF to location but for cross it is problem and even bigger when we and to contain all code for the build on Windows and MAC in the QtRvSim source tree. |
Dear @mgschossmann, I have spent some time with @fvacek to discuss his opinion and shares my feeling that Qt CLI application should be structured the same as the regular C/C++ command line application. That is the command line options should be parsed before application main even loop is started and errors during parsing should finish application through regular So I have ensured that I have not t applied Another not applied enhancement is You can see updated series at branch https://github.com/cvut/qtrvsim/commits/cli-updates-revided/ If there are no objections from you or others I will merge this to mainline and then consider the PC and format changes. |
Thanks @ppisa for reviewing my commits and integrating most of them. Regarding Analogously also Finally, regarding the deferred So all in all, I have no objections to merging this branch to mainline. Regarding the gdbserver: For the purpose of our homework testing, it provies all necessary features. For inclusion in QtRVSim, I think it is still lacking completeness. Especially, I only wrote it with the single-cycle processor in mind and never tested compatibility with the pipelined processor. I expect there to be bugs related to register writes from gdb. Also, I only checked gdb compatibility during development and then used an own Python-library to communicate with QtRVSim. I remember that I had not yet managed to make The gdbserver is currently only available from the CLI and only, if a tcp port or a filename for an unix socket is provided as a command line argument. I however think, that integration from the GUI should not be too difficult. If I find time, I plan to rebase the commits onto |
OK, I have pushed changes. As I think again about startup address, I see its proper location in the config and it should be shown above Elf executable (or may be below) as Another approach how to specify start address or start symbol from sources for internal assembly would be some pragma
As for GDB, get into sync when pipeline is running underneath is almost impossible. It is the same as for real targets, I have lot of experience with JTAG and I have cooperated on the first GDB BDM support for m68k/CPU32. So the operation in QtRvSim has to be solved same way as on the real CPU. When asynchronous request to stop is issued (i.e. Ctrl+C in GDB) it has to be recorded with fetched instruction and real stop has to be done when instruction reaches commit stage (mem in our case) and preceding one write back has to be finished. For hard breakpoints (which are realized by ticks in Bp column) it has to work the same way. When fetched marked instruction, it has to be allowed to reach mem phase. If it is break instruction it is marked in decoder. Fetch is stopped then and if there is something in fetch or any lower number stage before marked instruction then it could be optimized to clean as instruction propagates (this logic is there already for device interrupts, syscalls and exceptions). All stages have to be flushed when the marked instruction propagates to the mem stage and write-back of proceeding one is finished. In this state, the architectural state matches registers state and it is possible even manipulate with state/registers and then start again by filling empty pipeline from PC of exception causing instruction or next one in break instruction case. That is step request from GDB in pipelined configuration would usually result in five cycles. If it is only single step then after successful fetch of one instruction flowing one has to be marked as hard-break one. So again similar as on real HW. QtRvSim is written in C but it models real HW like in Verilog or VHDL, but by C code as combinational logic in stages. So if you have already invested into RISC-V GDB protocol/GDB stub re-implementation for QtRvSim than if we join the forces we should be able to solve it for all combinations. But my time is limited and I am not sure if there would be some more spare whiles before Christmas. By the way, we prepare for NuttX workshop now https://events.nuttx.apache.org/ It is held on Thursday and Friday and my colleagues present three of our mainlined contributions there. |
Closing because the commits have been revised/edited and applied onto master through the separate branch e4470d5 Thanks for the changes and ideas. |
These commits improve the CLI by fixing a bug, adding various small features and improving console output.
The first commit fixes a bug which causes QtRVSim to get stuck when an error occurs before the main loop starts. While the commit looks like a lot of added and removed lines, it is mostly refactoring, since I moved the code from cli/main.cpp into an own class, which resembles one instance of the CLI program. (In some sense, CLIMain is the CLI equivalent of the MainWindow class.) For a more detailed reasoning, see the commit message.
Newly added features include:
The remaining commits improve the format of console output. In particular, they added some missing newlines.
I can also split this PR into multiple PRs if you prefer it that way. Also, I can take out features if they do not align with the scope of QtRVSim.
I have also written code implementing a working gdbserver within QtRVSim, which we use at my university to control QtRVSim from an automated homework tester. If you are interested, I can also open a pull request for the gdbserver (but it may take me a while). However, the code is not as mature as these commits are (e.g. it only works for 32-bit RISC-V) and I think it is unlikely that there would be other users of such a feature.