Misleading dead code in a generated catch block

[gh2375]

ikos produces false positives with regard to dead code in destructors.

Minimal working example:

#include <cstdlib>

struct MyStruct
{
    MyStruct()
    {
        data = (char*)malloc(10);
    }

    ~MyStruct()
    {
        free(data);
    }

    char * data;
};

int main(int argc, char * argv[])
{
    MyStruct s;

    return 0;
}

Output:

# Results
../../../test-destructor.cpp: In function 'MyStruct::~MyStruct()':
../../../test-destructor.cpp:13:3: unreachable: code is dead
                free(data);
                ^
../../../test-destructor.cpp: In function 'MyStruct::~MyStruct()':
../../../test-destructor.cpp:13:3: unreachable: code is dead
                free(data);
                ^

System:

MSYS2

Version:

$ ./ikos --version
ikos 2.1

[arthaud]

Hi @gh2375,

Thanks for reporting this issue.

Actually, clang wraps the call to free(data) in a try/catch block. The llvm bitcode looks like:

~MyStruct() {
  try {
    free(data);
  } catch (...) {
    __clang_call_terminate();
  }
}

The ikos report is actually about __clang_call_terminate() being dead code, but since this is not visible in the source code, the report is confusing.
I don't really know how we could improve this.

[MrSapps]

Given free can't throw.. is this not a clang/llvm codegen issue?

[arthaud]

Unfortunately, free can throw. See https://stackoverflow.com/questions/2486386/why-do-i-see-throw-in-a-c-library

Since ikos doesn't handle multithreading and pthread, it assumes that free doesn't throw.

[arthaud]

I still don't know how to better handle this situation. Suggestions are welcome.

[gh2375]

With regard to free:

Unfortunately, 'free' can throw. See https://stackoverflow.com/questions/2486386/why-do-i-see-throw-in-a-c-library

I am not entirely sure if I understand that Stack Overflow thread correctly. The last answer says that __THROW is defined as __attribute__((__nothrow__)), the second to last answer says that it is a cancellation point for the pthreads library, but I do not think that is a C++ exception. cplusplus.com says:

No-throw guarantee: this function never throws exceptions.
http://www.cplusplus.com/reference/cstdlib/free/

LLVM is generating a landingpad and a terminate call whenever a function is called in a destructor, except when that function has the nounwind attribute. A function gets that nounwind attribute if it is declared __attribute__((__nothrow__)), noexcept, throw(), etc.

One thing I saw was that with MSYS2 one gets a unreachable code warning, whereas on Debian 9 that is not the case. Looking into the respective C standard libraries one sees:

mingw-64:
void __cdecl free(void *_Memory);

glibc:
# define __THROW __attribute__ ((__nothrow__ __LEAF))
extern void free (void *__ptr) __THROW;

So on MSYS2 LLVM sees that free is possibly throwing, thus generates a landingpad and ikos detects the dead code. On Debian 9 LLVM sees that free is non-throwing, thus generates no landingpad and ikos detects no dead code.

However, I think the problem is more general than this. Already the following example

int f1();

struct MyStruct
{
    ~MyStruct()
    {
        f1();
    }
};

int main(int argc, char * argv[])
{
    MyStruct s;

    return 0;
}

yielding

; Function Attrs: noinline nounwind uwtable
define linkonce_odr dso_local void @_ZN8MyStructD2Ev(%struct.MyStruct*) unnamed_addr #2 comdat align 2 personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) !dbg !405 {
  call void @llvm.dbg.value(metadata %struct.MyStruct* %0, metadata !406, metadata !DIExpression()), !dbg !408
  %2 = invoke i32 @_Z2f1v()
          to label %3 unwind label %4, !dbg !409

; <label>:3:                                      ; preds = %1
  ret void, !dbg !411

; <label>:4:                                      ; preds = %1
  %5 = landingpad { i8*, i32 }
          catch i8* null, !dbg !409
  %6 = extractvalue { i8*, i32 } %5, 0, !dbg !409
  call void @__clang_call_terminate(i8* %6) #6, !dbg !409
  unreachable, !dbg !409
}

will trigger the unreachable code warning.

The behavior of ikos is correct in this case. The core problem from a user point of view is that in C++ destructors are very commonly used and ikos' behavior leads to a lot of noise which furthermore is in this case not of any help to the programmer, as the programmer cannot influence this specific part of the source code, as the programmer has not written it in the first place.

So my suggestion would be: If LLVM is generating special code (inserting a call to __clang_call_terminate) in a special case (an exception would leave the scope of a destructor), one could handle it as a special case. In particular that would mean:

• Detect that this function is a destructor. In the example above we have
  !405 = distinct !DISubprogram(name: "~MyStruct", linkageName: "_ZN8MyStructD2Ev", scope: !397, file: !1, line: 10, type: !400, isLocal: false, isDefinition: true, scopeLine: 11, flags: DIFlagPrototyped, isOptimized: false, unit: !0, declaration: !399, retainedNodes: !2)
  One could check that the first character of name is ~.
• Detect that the dead code is a basic block with a call to __clang_call_terminate
• Silence the dead code warning if the user wants to (i.e. introduce new command-line parameter)

[arthaud]

Regarding free: It's not entirely clear to me either.

It looks like this is an implementation detail in glibc for thread cancellation, see man7.org.
Apparently these exceptions have a specific behavior and can cause problems, see this stackoverflow question.
It is also surprising that cplusplus.com states that free doesn't throw but cppreference.com doesn't mention anything.

On my work laptop (macOS High Sierra), free is not declared nothrow. I'm also getting the unreachable: code is dead message.

---

Regarding the example: Actually, since f1 is not declared nothrow, it can throw.

The call to __clang_call_terminate is reachable, and ikos knows it.
ikos complains that the unreachable statement after call void @__clang_call_terminate is unreachable. This is silly, since in that case there is no code to run after this statement.

---

Regarding your suggestion: I agree, but it will be a bit tricky to implement.

[ivanperez-keera]

It looks like this is a matter for further discussion, and there's no agreement on whether IKOS is reporting this correctly or not (can free throw or not). I'm moving this to a discussion for now, and we can revisit when we have a clear plan in the future.

Please feel free to continue the discussion. Thanks!