Skip to content

Stop FuncSim on trap hit #601

Merged
merged 10 commits into from
Oct 4, 2018
Merged

Conversation

trexxet
Copy link
Contributor

@trexxet trexxet commented Oct 4, 2018

Breakpoint, syscall and other traps stops FuncSim execution (as I did it in #557).

@trexxet
Copy link
Contributor Author

trexxet commented Oct 4, 2018

Actually, Simulator::StopReason may be merged inito Trap, because Trap is set only with StopReason::TrapHit. However, I think it doesn't look nice, because stop reason logically is not a trap.

@pavelkryukov
Copy link
Member

However, I think it doesn't look nice, because stop reason logically is not a trap.

There is no such entity as 'stop reason', Halt is just a one of the trap types.

However, I think it doesn't look nice,

std::pair is much uglier, especially if you are C++ newbie.
Let's keep the things simple.

@codecov
Copy link

codecov bot commented Oct 4, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@0f0fa26). Click here to learn what that means.
The diff coverage is 92%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #601   +/-   ##
=========================================
  Coverage          ?   91.22%           
=========================================
  Files             ?       46           
  Lines             ?     1550           
  Branches          ?        0           
=========================================
  Hits              ?     1414           
  Misses            ?      136           
  Partials          ?        0
Impacted Files Coverage Δ
simulator/func_sim/func_sim.h 66.66% <ø> (ø)
simulator/modules/core/perf_sim.h 50% <ø> (ø)
simulator/risc_v/riscv_instr.h 0% <0%> (ø)
simulator/mips/mips_instr.h 94.44% <100%> (ø)
simulator/mips/mips_instr.cpp 90.06% <100%> (ø)
simulator/modules/core/perf_sim.cpp 100% <100%> (ø)
simulator/simulator.h 100% <100%> (ø)
simulator/func_sim/func_sim.cpp 93.75% <75%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f0fa26...086ab06. Read the comment docs.

@@ -450,7 +446,9 @@ class BaseMIPSInstr

bool is_special() const { return operation == OUT_R_SPECIAL; }

bool has_trap() const { return trap != TrapType::NO_TRAP; }
bool has_trap() const { return trap != Trap::NO_TRAP; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use functionality of other methods, i.e.

return trap_type() != Trap::NO_TRAP;

@@ -0,0 +1,21 @@
/* trap_types.h - Trap types for MIPS and RISC-V
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That file should be moved to.... FuncSim directory, I believe.
In my opinion, infra should contain generic things, unrelated to system and ISA stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it's used by mips_instr and riscv_instr. If we had a base ISA-independent class for instruction, it would fit well in it. Also, it can be moved to trap implementation.

Copy link
Contributor Author

@trexxet trexxet Oct 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I see now, it should be moved to trap class. At this moment, should it be moved to FuncSim directory, or I can leave it as is?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep "everything related to functional state" in func_sim: RF, system calls, trap handler etc.. Later we'll use better name, I believe.

@pavelkryukov pavelkryukov mentioned this pull request Oct 4, 2018
@@ -64,15 +64,19 @@ void FuncSim<ISA>::init( const std::string& tr)
}

template <typename ISA>
void FuncSim<ISA>::run( const std::string& tr, uint64 instrs_to_run)
Simulator::RunResult FuncSim<ISA>::run( const std::string& tr, uint64 instrs_to_run)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, it's worth to return FuncInstr here....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this moment, it would be hard to return FuncInstr from PerfSim. I think we may change it later, when traps would be implemented in PerfSim.

pavelkryukov
pavelkryukov previously approved these changes Oct 4, 2018
@pavelkryukov pavelkryukov merged commit 8788425 into MIPT-ILab:master Oct 4, 2018
@trexxet trexxet deleted the break_trap_instr branch October 4, 2018 17:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants