avast / retdec-idaplugin

RetDec plugin for IDA
https://retdec.com/
MIT License
762 stars 129 forks source link

Please support IDA 7.7 #68

Closed Naville closed 2 years ago

Naville commented 2 years ago

Hey:

I had to apply a bunch of patches for it to even compiles with IDA7.7, which you can use as a reference here

Although, getting its functionality to work (which broke for some reason) is beyond me

hugmyndakassi commented 2 years ago

At least your replacement of inf_is_32bit is wrong. It should now be inf_is_32bit_or_higher(). Please check out the comment above its definition:

// unfortunately the name inf_is_32bit was used in the past to mean inf_is_32bit_or_higher.
// it was misleading, this is why we have more explicit names now.

Not sure if it would fix the issue, though.

A cursory look at the IDA 7.5 SDK shows that CM_CC_GOLANG used to be called CM_CC_RESERVE4. So this could be addressed at the preprocessor level. But it would have been nice to keep the old name around for backward compatibility, if that's the case. Even if it were something like:

#ifdef IDA_SDK_DEPRECATED
#define CM_CC_RESERVE4 CM_CC_GOLANG
#endif

I never understood why the IDA SDK isn't publicly available as a Git repo. Heck, even just for those with active maintenance would be possible. But I suppose having a Git repo of sorts would make it likelier that it "slips out" so that seems to be the main concern on part of Hex-Rays. But these incompatibilities have come up so many times, it's tiring.

At least with IDA_SDK_VERSION we have some (very limited) ability to check against the version and put our own compatibility glue around it, but some changes make it harder than others to stay source compatible. Well, there's NO_OBSOLETE_FUNCS but it seems to have fallen out of use or something.

hugmyndakassi commented 2 years ago

The original code here amounts to:

    if (true) bitSize = 64;
    // else if (true) bitSize = 32;

... because the address of inf_is_64bit and inf_is_32bit will be non-null. This is very very strange code indeed and likely not the intention of the author. Probably not covered by tests?

This is more likely a reason why something malfunctions.

My patch ended up as follows:

diff --git a/src/idaplugin/config.cpp b/src/idaplugin/config.cpp
index 776dbe6..8606489 100644
--- a/src/idaplugin/config.cpp
+++ b/src/idaplugin/config.cpp
@@ -34,7 +34,7 @@ bool canDecompileInput(
            return false;
        }
    }
-   else if (!inf_is_32bit())
+   else if (!inf_is_32bit_or_higher())
    {
        WARNING_GUI(RetDec::pluginName << " version " << RetDec::pluginVersion
                << " cannot decompile PROCNAME = " << procName
@@ -99,8 +99,8 @@ bool canDecompileInput(
    //
    if (fileType == f_BIN)
    {
-       if (inf_is_64bit) bitSize = 64;
-       else if (inf_is_32bit) bitSize = 32;
+       if (inf_is_64bit()) bitSize = 64;
+       else if (inf_is_32bit_or_higher()) bitSize = 32;
        else
        {
            WARNING_GUI("Can decompile only 32/64 bit f_BIN.\n");
@@ -552,7 +552,7 @@ void generateCallingConvention(

        case CM_CC_INVALID:
        case CM_CC_UNKNOWN:
-       case CM_CC_RESERVE4:
+       case CM_CC_GOLANG:
        case CM_CC_RESERVE3:
        default:             configCC.setIsUnknown(); break;
    }
diff --git a/src/idaplugin/place.cpp b/src/idaplugin/place.cpp
index a910b14..3fc7e15 100644
--- a/src/idaplugin/place.cpp
+++ b/src/idaplugin/place.cpp
@@ -239,7 +239,7 @@ void retdec_place_t::registerPlace(const plugin_t& PLUGIN)
    ///
    /// Note: Whenever one of the 'p1' or 'p2' places is unregistered,
    /// corresponding converters will be automatically unregistered as well.
-   register_loc_converter(
+   register_loc_converter2(
        _template.name(),
        _idaplace.name(),
        place_converter
@@ -287,7 +287,7 @@ std::ostream& operator<<(std::ostream& os, const retdec_place_t& p)
 lecvt_code_t idaapi place_converter(
         lochist_entry_t* dst,
         const lochist_entry_t& src,
-        TWidget* view)
+        TWidget* view, uint32)
 {
    // idaplace_t -> retdec_place_t
    if (src.place()->name() == std::string(_idaplace.name()))
diff --git a/src/idaplugin/place.h b/src/idaplugin/place.h
index ca34dea..0bd2e25 100644
--- a/src/idaplugin/place.h
+++ b/src/idaplugin/place.h
@@ -219,7 +219,8 @@ class retdec_place_t : public place_t
 lecvt_code_t idaapi place_converter(
         lochist_entry_t* dst,
         const lochist_entry_t& src,
-        TWidget* view
+        TWidget* view,
+        uint32 flags
 );

 #endif

Same thing zipped up and attached: idasdk77-fixups.zip

angrygoats commented 2 years ago

Seconding in case the authors are monitoring issues. Would like to see IDA 7.7 officially support. Will try this patch later today. Thank you @hugmyndakassi

nkAlex commented 2 years ago

Hey, Could anyone please also share the fixed compiled binaries?

PeterMatula commented 2 years ago

I will try to find time next week to update the sources to the latest IDA and do the release.

PeterMatula commented 2 years ago

Thanks, @hugmyndakassi I looked into it and your changes are pretty much all that's needed to get it to work - I didn't encounter new problems that wouldn't be there before. I will try to do the binary release as soon as I get a colleague with a Windows IDA license.

While I'm at it, I will also try to get it to work on IDA 8.0 but we will see how big of a problem it will be.

PeterMatula commented 2 years ago

Looks like 8.0 compiles and works with the code for 7.7. But again, I will have to get my hands on the Windows license (and Windows, and MSVC :-D) to make the pre-built package.

PeterMatula commented 2 years ago

Two more releases done: v1.0-ida77 and v1.0-ida80.