i#6238: validate instructions: push,pop,cmp,pusha,popa,push_imm,ret,ret_far,loopne,loope,loop,movzx,cmp,rcr

[kuhanov]

Add instructions to categories:
OP_push,OP_pop,OP_cmp,OP_pusha,OP_popa,OP_push_imm,OP_ret,OP_ret_far,OP_loopne,OP_loope,OP_loop,OP_movzx,OP_cmp,OP_rcr

Fix: Moved category decoding after operands decoding to check STORE/LOAD memory access

Issue: #6238

[derekbruening]

remove FP | MOVE if we change the default category to LOAD and/or STORE

You removed FP for LOAD/STORE? I think people would want to distinguish a load or store to an FP reg from a GPR.

[kuhanov]

remove FP | MOVE if we change the default category to LOAD and/or STORE

You removed FP for LOAD/STORE? I think people would want to distinguish a load or store to an FP reg from a GPR.

Hm. Let me add to new types for FP to eclude subcategory assert in this case.
Is it ok?

diff --git a/core/ir/instr_api.h b/core/ir/instr_api.h
index 995f7267c..366314ae9 100644
--- a/core/ir/instr_api.h
+++ b/core/ir/instr_api.h
@@ -1918,6 +1918,8 @@ typedef enum {
     DR_FP_MOVE,    /**< Moves floating point values from one location to another. */
     DR_FP_CONVERT, /**< Converts to or from floating point values. */
     DR_FP_MATH,    /**< Performs arithmetic or conditional operations. */
+    DR_FP_LOAD,    /**< Loads. */
+    DR_FP_STORE,   /**< Stores. */
 } dr_fp_type_t;
 
 DR_API
diff --git a/core/ir/x86/decode.c b/core/ir/x86/decode.c
index 78e074203..ca05ad645 100644
--- a/core/ir/x86/decode.c
+++ b/core/ir/x86/decode.c
@@ -2449,17 +2449,11 @@ decode_category(instr_t *instr)
             if (instr_operands_valid(instr)) {
                 if (instr_reads_memory(instr)) {
                     category |= DR_INSTR_CATEGORY_LOAD;
-                    if (TEST(DR_INSTR_CATEGORY_MOVE, category)) {
-                        category &= ~DR_INSTR_CATEGORY_MOVE;
-                        category &= ~DR_INSTR_CATEGORY_FP;
-                    }
+                    category &= ~DR_INSTR_CATEGORY_MOVE;
                 }
                 if (instr_writes_memory(instr)) {
                     category |= DR_INSTR_CATEGORY_STORE;
-                    if (TEST(DR_INSTR_CATEGORY_MOVE, category)) {
-                        category &= ~DR_INSTR_CATEGORY_MOVE;
-                        category &= ~DR_INSTR_CATEGORY_FP;
-                    }
+                    category &= ~DR_INSTR_CATEGORY_MOVE;
                 }
             }
             instr_set_category(instr, category);
diff --git a/core/ir/x86/instr.c b/core/ir/x86/instr.c
index 618adba98..a08b51496 100644
--- a/core/ir/x86/instr.c
+++ b/core/ir/x86/instr.c
@@ -930,6 +930,14 @@ instr_is_floating_ex(instr_t *instr, dr_fp_type_t *type OUT)
         if (type != NULL)
             *type = DR_FP_MOVE;
         return true;
+    } else if (TEST(DR_INSTR_CATEGORY_STORE, cat)) {
+        if (type != NULL)
+            *type = DR_FP_STORE;
+        return true;
+    } else if (TEST(DR_INSTR_CATEGORY_LOAD, cat)) {
+        if (type != NULL)
+            *type = DR_FP_LOAD;
+        return true;
     } else {
         CLIENT_ASSERT(false, "instr_is_floating_ex: FP instruction without subcategory");
         return false;

[derekbruening]

I don't think we should be making compound categories like FP_MOVE, FP_STORE, or FP_LOAD when we can compose the existing categories: what is the advantage of FP_STORE over FP|STORE? I would think combining FP with the other categories is what we want.

[kuhanov]

I don't think we should be making compound categories

Let's remove dr_fp_type_t at all in this case and migrate to categories.
I could cteate patch tommorow.
Do you agree?
Kirill

[derekbruening]

Oh I misread it: I thought that patch in your comment #6331 (comment) was adding DR_INSTR_CATEGORY_FP_LOAD. I don't think we need to update the older dr_fp_type_t. Marking it as deprecated (there is a Doxygen tag for that) sounds fine.