Revisit, assess and extend ETISS Fault Injection #100

JoGei · 2021-11-24T11:33:52Z

This PR revisits the existing fault injection functionality in ETISS and extends it.

rafzi

looks good to merge, just some comments

rafzi · 2021-11-26T10:05:13Z

src/ETISS.cpp

+        if (!faults.empty())
+        {
+            std::list<std::string> ffs = etiss::split(faults, ';');
+            for (auto ff : ffs)


The default pattern for this should be "const auto&". If you need mutablity, drop const and if you need a copy, drop the reference.

agreed (copied from old code base). Fixed with 3087cae

rafzi · 2021-11-26T10:06:24Z

src/ETISS.cpp

+            }
+
+            etiss::log(etiss::VERBOSE, std::string("Add InstructionAccurateCallback Plugin to ") + cpu_core->getName() + std::string(". Required for etiss::fault::Injector."));
+            cpu_core->addPlugin(std::shared_ptr<etiss::Plugin>(new etiss::plugin::InstructionAccurateCallback()));


avoid explicit "new" by using "make_shared"

agreed (copied from example/bare_etiss_processor/main.cpp:L129). Fixed with 5149bc1 - also addresses example/bare_etiss_processor/main.cpp

rafzi · 2021-11-26T10:11:07Z

src/IntegratedLibrary/fault/MemoryManipulationSystem.cpp

+
+std::shared_ptr<VirtualStruct> MemoryManipulationSystem::getStruct(void)
+{
+    if(!vsystem_)


This file and others do not follow our coding style. please use the provided .clang-format configuration

Was not aware of clang format style. Used the following before committing changes:

clang-format --style=file --verbose src/fault/Fault.cpp src/fault/Action.cpp include/etiss/fault/Action.h \ include/etiss/fault/Fault.h include/etiss/fault/Trigger.h src/fault/Trigger.cpp src/fault/Stressor.cpp \ include/etiss/fault/Injector.h src/VirtualStruct.cpp include/etiss/VirtualStruct.h src/fault/InjectorAddress.cpp \ include/etiss/fault/InjectorAddress.h include/etiss/fault/Stressor.h src/fault/Injector.cpp -i

rafzi · 2021-11-26T10:19:41Z

src/IntegratedLibrary/fault/MemoryManipulationSystem.cpp

+            split_cmd.push_back(cmd);
+            if (split_cmd.size() > 1)
+            {
+                std::stringstream ss;


std::stoll would make this less verbose and more robust

would prefer boost::lexical_cast over std::sto*, but either over std::stringstream. Fixed with 459ac53

rafzi · 2021-11-26T10:22:31Z

src/VirtualStruct.cpp

+        find = fieldPrettyNames_.find(field);
+        if (find == fieldPrettyNames_.end())
+        {
+            f = 0;


is already set

hello copy paste my old friend... Fixed with c8f9478

not sure if that fix is correct. the line you removed was searching a different list. what i meant is that f is already initialized to null at the start of the function, so this assignment is redundant and the conditional block can be dropped.

you are right. reversed and fixed with c86cb7f

rafzi · 2021-11-26T10:27:54Z

src/fault/Action.cpp

 {
    return type_;
 }

 const InjectorAddress &Action::getInjectorAddress() const
 {
+    if(unlikely(!(type_ == Type::BITFLIP || type_ == Type::MASK || type_ == Type::COMMAND)))


unless they are measurably important for performance, i'd perfer to avoid the un/likely clutter

agreed. From old code base, but the getters are not performance critical. Fixed with b6da677

rafzi · 2021-11-26T10:33:15Z

src/fault/Trigger.cpp

 Trigger::Trigger(const Trigger &sub, uint64_t count)
-    : type_(META_COUNTER), sub_(new Trigger(sub)), param1_(count), param2_(0)
+  : type_(Type::META_COUNTER)
+  , sub_(new Trigger(sub))


should be unique_ptr + make_unique

agreed. Also old code base not really up to date with current coding style. Fixed with 909d74e and 746b340 (remove wild inclusion style)

rafzi · 2021-11-26T10:36:08Z

src/fault/Trigger.cpp

+  : type_(Type::META_COUNTER)
+  , sub_(new Trigger(sub))
+  , param1_(count)
+  , param2_(0)


there are many uninitialized fields in these constructors. maybe just default initialize them in the class definition

Fixed with 909d74e

rafzi · 2021-12-03T20:22:51Z

include/etiss/fault/Action.h

-
+  private:                                            // Attributes
+    type_t type_;                                     ///< type of the Attribute
+    std::unique_ptr<InjectorAddress> inj_{ nullptr }; ///< Address of Injector


for unique_ptr and string the initialization is redundant

rafzi · 2021-12-03T20:52:58Z

include/etiss/fault/Misc.h

+////////////////////////////////////////////////////////////////////////////////////////////////
+/// \brief Action type class
+template <typename enum_t>
+class SmartType : public etiss::ToString


this seems nice, but as implemented a bit weak: you still have to manually set up the table causing code duplication and potential for errors. the error handling is inconsistent between from/toString - one returns bool, the other throws. a lot of code could be constant evaluated. outdated docstrings

i think it makes sense to just use an existing solution for this problem. for example i found this: https://github.com/aantron/better-enums

Sure, I understand these concerns. I had this laying around for when I needed string->enum and enum->string encoding as we need here for all the from and to xml stuff going on in ETISS-FI.
The better-enums seems perfect, but would need a third party integration of it in ETISS, which I would like to avoid in this PR. I'd even suggest to add a /third_party/ directory to ETISS root to easier integrate such things - either through git submodules or CMake fetch. Otherwise we end up with missing copyright/licenses and pasted source code like we already have here:

https://github.com/tum-ei-eda/etiss/tree/master/src/fault/xml

https://github.com/tum-ei-eda/etiss/tree/master/src/fpu

https://github.com/tum-ei-eda/etiss/tree/master/src/IntegratedLibrary/SimpleIni
just some that I know of...

fixed with 5e795d7

rafzi · 2021-12-03T20:54:39Z

include/etiss/fault/Fault.h

+    Fault(int nullid);
+    Fault(const Fault &cpy);
+    Fault &operator=(const Fault &cpy);
+#if CXX0X_UP_SUPPORTED


these can be removed, because we now support c++14

rafzi · 2021-12-03T20:55:43Z

include/etiss/fault/Injector.h

-    volatile bool has_pending_triggers;
-    std::list<std::pair<Trigger, int32_t>> pending_triggers; ///> Triggers which were just added
-    std::list<std::pair<Trigger, int32_t>> unknown_triggers; ///> Triggers to look at in callbacks
+    volatile bool has_pending_triggers{false};


why are these volatile? if thread-safety is intended, this will not work. an atomic would be required

I do not know either. I removed them for now fba85d5

rafzi · 2021-12-03T21:06:19Z

src/VirtualStruct.cpp

+        find = fieldPrettyNames_.find(field);
+        if (find == fieldPrettyNames_.end())
+        {
+            f = 0;


not sure if that fix is correct. the line you removed was searching a different list. what i meant is that f is already initialized to null at the start of the function, so this assignment is redundant and the conditional block can be dropped.

rafzi · 2021-12-03T21:20:21Z

src/fault/Action.cpp

+    switch (type_)
+    {
+    case Type::BITFLIP:
+        inj_ = std::make_unique<InjectorAddress>(cpy.getInjectorAddress());


it is not clear to me why you made inj_ pointers. could they not just be value types so that this copy becomes a trivial "=default"?

Unfortunately not so easily.
For example in Stressor::firedTrigger:

for (std::vector<etiss::fault::Action>::iterator iter = find->second.actions.begin(); iter != find->second.actions.end(); ++iter) { ... default: // on field actions { if (iter->getInjectorAddress().getInjector())

The inj_ member of Action is used to decide whether the Action may run through this Injector or is legal in the first place. The check is whether the member was set. Without major changes to most of the old fault injection stuff, I can not re-use most of it. All this FI code is way older than this open source repository. I do not even know who wrote it initially.

The idea behind this PR was to make the FI usable again, seems to take a lot more work to make the code up to date than the functionality.

The Action and Trigger classes both are built similar in that their instances have uninitialized members depending on their respective type_. Obviously, this could be done much better through different design patterns, but such major changes were not initially the idea behind this PR. If you think such a refactoring process is a must, I can start with it.

rafzi · 2021-12-03T21:26:30Z

src/fault/Fault.cpp

+
+Fault::Fault(int nullid) : id_(nullid) {}
+
+Fault::Fault(const Fault &cpy)


these copy/move constr/assign are equivalent to the default

Yes. Was not aware of the main CMakeLists.txt already forcing c++14
eff549b

rafzi · 2021-12-03T21:41:04Z

src/fault/Injector.cpp

+#if CXX0X_UP_SUPPORTED
+        std::lock_guard<std::mutex> lock(sync);
+#endif
+        for (const auto &rm : remove_triggers)


could use erase-remove idiom

auto shouldRemove = [&](auto elem){ return std::find(b.begin(), b.end(), elem) != b.end(); }; a.erase(std::remove_if(a.begin(), a.end(), shouldRemove), a.end());

otherwise the typical pattern uses a while loop and iter=a.erase(iter);

rafzi · 2021-12-07T10:49:30Z

thanks for the justifications, but please don't feel obligated to address all of my comments. as i said, this would have been fine to merge on the first iteration. just noted some stuff that caught my eye

… of VirtualStruct require it being mounted with an instance of CPUCore::getStruct(). Move 'faults.xml' configuration resolve from etiss::initialize to initialize_virtual (requires mounted VirtualStruct). Add 'faults.xml' configuration as command line parameter. Make VirtualStruct::Field::flags_ member a non-const to allow runtime configuration of access type.

…d), but are discarded.

…ger[lock mutex]->Stresser::addFault[lock mutex]

…initialize_virtualstruct call. Add WIP: Fault Injection How-To to examples/bare_etiss_processor/README.md and example `fault.xml` file for better reference.

… action for CPUCore's VirtualStruct

… fault injection flag of a field will be set if a fault for the field is configured through Stressor. Add "MASK" fault as Action. A Mask fault has a mask operation <op> (and, or, xor, nand, nor) and a mask value <value> (64-bit) acting on a <field>: "<field> = <field> <op> <value>"

…om xml). Add event type to <action> types which allows to inject ETISS simulation events (all ETISS::RETURNCODES), e.g., to flush translation cache. Minor fixes in mask type <action>

…instructionAccurateCallback() to trigger on next trigger check. Update Trigger typing code.

…ication of mounted structs. Minor typo fixes.

…actions from fault injection. Extends SimpleMemSystem.

…nique_ptr, add missing copy constructors.

…ulars.

…xplicitly added through a fault's INJECTION action or through the <initial> node in given XML, add EJECTION action that deactivates all triggers of the referenced fault, disable automatic removal of a trigger once it was fired to allow permanent faults - remove triggers (therefore the fault) through EJECTION action of a referenced fault, extend faults.xml example and readme section w.r.t. new features.

… version

rebase from master

…ple instances insert callback within same codeblock

JoGei mentioned this pull request Nov 24, 2021

Issues with Existing Fault Injection Code #99

Open

13 tasks

JoGei changed the title ~~Revisit, asses and extend ETISS Fault Injection~~ Revisit, assess and extend ETISS Fault Injection Nov 24, 2021

rafzi reviewed Nov 26, 2021

View reviewed changes

rafzi reviewed Dec 3, 2021

View reviewed changes

JoGei added 25 commits June 30, 2022 12:12

fix tab to space

b510cc5

Allow Trigger::NOP and Action::NOP both can be configured (or injecte…

fbfd3b8

…d), but are discarded.

fix deadlock when Action::INJECTION is results in Stressor::firedTrig…

c0c4b0c

…ger[lock mutex]->Stresser::addFault[lock mutex]

Update example/bare_etiss_processor with new CPUCore instance forced …

d0c70df

…initialize_virtualstruct call. Add WIP: Fault Injection How-To to examples/bare_etiss_processor/README.md and example `fault.xml` file for better reference.

add *.xml example to examples cmake install target

54c0249

update README "PC" is called "instructionPointer" in ETISS

3c005fd

overload initialize_virtualstruct to allow direct setting of a custom…

58f6822

… action for CPUCore's VirtualStruct

Add smart static mapping of types to their string representatives (fr…

fad029c

…om xml). Add event type to <action> types which allows to inject ETISS simulation events (all ETISS::RETURNCODES), e.g., to flush translation cache. Minor fixes in mask type <action>

Add as soon as possbile (ASAP) <trigger> type which forces Injector::…

e75be13

…instructionAccurateCallback() to trigger on next trigger check. Update Trigger typing code.

Add getName() pure abstract to VirtualStructSupport for easier identf…

724c396

…ication of mounted structs. Minor typo fixes.

Add MemoryFaultSystem that allows memory manipulation through custom …

edba074

…actions from fault injection. Extends SimpleMemSystem.

remove trailing whitespaces

0185fe5

replace "or" with primary token. Add parentheses in if()

ef00af6

use std::stoll instead of stringstream if number base is known

db116a0

apply clang format to fault injection code base

79411fb

force default initialization of enum variable

42fb71d

add default initializers to member variables, replace pointers with u…

86bf7b5

…nique_ptr, add missing copy constructors.

replace inclusions with forward declarations in headers to avoid circ…

bf6bfe1

…ulars.

use const iterator in faults config

9dfc92d

avoid new operator for shared_ptr

6564349

clean-up repetitive assignment

dc1acb3

remove builtin_expect macros for performance uncritical branching

cc1f2c2

JoGei added 5 commits June 30, 2022 12:14

fix Field pointer

a6bc78e

remove empty default initializations

7526b08

memory manipulation system for bare etiss processor in case of fi

446d7c3

rename Trigger check function to clarify usage

343042e

adapt fi examples to new build configuration

1d6f2d7

JoGei force-pushed the rev_faultinjection branch from b57f159 to 1d6f2d7 Compare June 30, 2022 11:08

JoGei added 3 commits December 16, 2022 08:57

Merge branch 'tum-ei-eda:master' into rev_faultinjection

d60635c

move pugixml to CMake FetchContent, update pugixml to cmake supported…

6d57eb2

… version

replace custom smart enum types with safer third party implementation

5e795d7

JoGei marked this pull request as ready for review December 16, 2022 13:21

JoGei mentioned this pull request Dec 16, 2022

Better integration of third party libraries #101

Open

4 tasks

JoGei added 19 commits December 16, 2022 14:50

explicit constructor for MemOp ub manipulator base for clang

dbc0914

refactor pugixml integration to avoid dependency in installed etiss

6c17d12

force command to upper case for enum resolve

40851ec

fix typo in error message

7f5fa60

Merge branch 'tum-ei-eda:master' into rev_faultinjection

a25a699

Merge branch 'rev_faultinjection' into rebaser

d225750

Update CMakeLists.txt

0566f5e

Merge pull request #3 from JoGei/rebaser

3ae4db7

rebase from master

Merge branch 'master' into rev_faultinjection

5510d17

update elfio

5d0d8ae

typos

4fae67f

add missing CSRs and interrupt vector to legacy RISCV-32bit Arch

861256f

make VirtualStruct inheritable

bd0835c

allow naming cpu cores other than "core+<id>"

209745b

check if VirtualStruct is already mounted to root

31f8610

bugix fault actions NOP and EVENT to correctly return status

92e851c

add unique ID to instruction specific callback plugin, to allow multi…

8fd276a

…ple instances insert callback within same codeblock

revert changes

f8bf2d6

Merge branch 'tum-ei-eda:master' into rev_faultinjection

186e0f4


		Fault::Fault(int nullid) : id_(nullid) {}

		Fault::Fault(const Fault &cpy)

Revisit, assess and extend ETISS Fault Injection #100

Are you sure you want to change the base?

Revisit, assess and extend ETISS Fault Injection #100

Conversation

JoGei commented Nov 24, 2021

rafzi 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoGei Dec 6, 2021 • edited Loading

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

rafzi commented Dec 7, 2021

JoGei Dec 6, 2021 •

edited

Loading