C goto crosses initialization, MinGW-W64 vs MSVC++

Question

I found here that according to the C++ standard:

It is possible to transfer into a block, but not in a way that bypasses declarations with initialization.

The C style of coding in my workplace (and by that I mean that other than the file extension, everything is practically C - no exceptions, no templates, no actual classes) obeys the rules specified here, namely exiting from a function in only one place, performing transfer of ownership only when reaching the full-success flow, performing cleanup on locals whose ownership we haven't transferred, etc. Here's a small example (error-code enumeration and other things omitted for brevity):

int func()
{
    int iStatus = -1;
    PVOID pvSomeData = NULL;
    HANDLE hFile = INVALID_HANDLE_VALUE;

    pvSomeData = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, BUFFER_SIZE);
    if (nullptr == pvSomeData)
    {
        iStatus = 1;
        goto lblCleanup;
    }

    const PTSTR pszFilePath = _T("C:\\temp\\bla.txt");
    _tprintf(_T("Writing some stuff into '%s'"), pszFilePath);

    hFile = CreateFile(pszFilePath, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
    if (INVALID_HANDLE_VALUE == hFile)
    {
        iStatus = 2;
        goto lblCleanup;
    }

    // do more stuff, fill pvSomeData, write it to the file, transfer ownership, etc.

    // success
    iStatus = 0;

lblCleanup:
    if (INVALID_HANDLE_VALUE != hFile)
    {
        CloseHandle(hFile);
    }
    if (nullptr != pvSomeData)
    {
        HeapFree(GetProcessHeap(), 0, pvSomeData);
    }

    return iStatus;
}

This C-style code compiles well under MSVC++, but when attempting to compile it under MinGW-W64, the compiler bemoans the issue of the goto statement crossing the initialization of pszFilePath. From the first link I can see that this would be well formed if I separated the initialization into a declaration and an assignment, since PWSTR is a POD type.

I'd like to compile my project under MinGW-W64 without performing extensive modifications to my codebase. Is there a way for me to do so (compiler flag, hopefully)?

I'm well aware of the idiomatic way to code this in C++ with RAII classes, but the codebase is flat C-style linear code. I'm also well aware of the strong aversion goto evokes in developers, and of the fact that any piece of code with a goto can be written without one. I ask not for style guidance, but for a least-effort way to solve this very specific problem.

Turn up your warnings, MSVC will warn you about it to. I'm pretty sure it shows up at W3 so you must have warnings turned down. BTW since it's undefined behavior you may experience [time travel](http://blogs.msdn.com/b/oldnewthing/archive/2014/06/27/10537746.aspx) — Mgetz, Nov 18 '14 at 22:41
@DoomMuffins - In the world of C++, there are `exceptions`. If an exception is thrown, your code will not recover using these techniques. You will have an open file handle and a memory leak if that occurs, and no amount of rearranging the code above will help that. I'm just letting you know that using RAII is the way to write this correctly. — PaulMcKenzie, Nov 18 '14 at 22:57
Perhaps I should tag this as C, rather than C++ - while the compiler is C++-aware, the code is very C-oriented: return codes, no exceptions, etc. — DoomMuffins, Nov 18 '14 at 23:06
Why not hoist the initialization of `pszFilePath` to the beginning of the function? — EOF, Nov 18 '14 at 23:38

score 3 · Answer 1 · answered Nov 18 '14 at 22:21

3

Probably the least painful way to fix things like this is to introduce braces to limit the scope of the problem variables, e.g. change this block:

const PTSTR pszFilePath = _T("C:\\temp\\bla.txt");
_tprintf(_T("Writing some stuff into '%s'"), pszFilePath);

hFile = CreateFile(pszFilePath, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
if (INVALID_HANDLE_VALUE == hFile)
{
    iStatus = 2;
    goto lblCleanup;
}

// do more stuff, fill pvSomeData, write it to the file, transfer ownership, etc.

// success
iStatus = 0;

to:

{
    const PTSTR pszFilePath = _T("C:\\temp\\bla.txt");
    _tprintf(_T("Writing some stuff into '%s'"), pszFilePath);

    hFile = CreateFile(pszFilePath, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
    if (INVALID_HANDLE_VALUE == hFile)
    {
        iStatus = 2;
        goto lblCleanup;
    }

    // do more stuff, fill pvSomeData, write it to the file, transfer ownership, etc.

    // success
    iStatus = 0;
}

answered Nov 18 '14 at 22:21

Paul R

208,748
37
389
560

1

Nice solution. If you really feel the need to use a `goto` that is! – Clifford Nov 18 '14 at 22:28
@Clifford: yes, ideally this kind of code needs to be cleaned up properly, but if you're faced with a legacy code base with lots of instances of this kind of sloppy programming then you probably just want the simplest/easiest/quickest solution. – Paul R Nov 18 '14 at 22:32
You can actually make the statement block the `else` of the preceding `if` to the same effect without making the code look "unusual" and making the first `goto` entirely redundant. – Clifford Nov 18 '14 at 22:35
@Claifford: true, in this instance - for the more general case though you usually just want to push the problem variables into their own local scope if possible. It depends on how much thought/effort you want to put into each occurrence of this kind of problem though - I've had to fix legacy code with hundreds of instances of this kind of thing before, so I tend to go for the 2 second fix rather than the 2 minute fix. ;-) – Paul R Nov 18 '14 at 22:40

score 1 · Answer 2 · answered Nov 18 '14 at 22:26

All the gotos in this function are trivially avoided:

int func()
{
    int iStatus = -1;
    PVOID pvSomeData = NULL;
    HANDLE hFile = INVALID_HANDLE_VALUE;

    if (nullptr == pvSomeData)
    {
        iStatus = 1;
    }
    else
    {
        const PTSTR pszFilePath = _T("C:\\temp\\bla.txt");
        _tprintf(_T("Writing some stuff into '%s'"), pszFilePath);

        hFile = CreateFile(pszFilePath, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
        if (INVALID_HANDLE_VALUE == hFile)
        {
            iStatus = 2;
        }
        else
        {

            // do more stuff, fill pvSomeData, write it to the file, transfer ownership, etc.

            // success
            iStatus = 0;
        }
    }

    if (INVALID_HANDLE_VALUE != hFile)
    {
        CloseHandle(hFile);
    }
    if (nullptr != pvSomeData)
    {
        HeapFree(GetProcessHeap(), 0, pvSomeData);
    }

    return iStatus;
}

1. Not all gotos in the codebase are trivially avoidable, at least in a way that preserves code clarity. 2. This reduces code clarity and linearity significantly. 3. This does not solve my problem. — DoomMuffins, Nov 18 '14 at 22:32
@DoomMuffins : 1) In 25 years in software development, I have never needed to use a goto in C or C++, and only once used legacy code where removing one was too much trouble. If you have many, then good luck with that! 2) I have to strongly disagree. 3) Really? It exactly solves the problem presented, it is unreasonable to expect a solution to situations you have not presented. However PaulR's solution is probably your bet bet. Otherwise you will have to move all initialised declarations to above *any* goto. Greater or lesser localisation. — Clifford, Nov 18 '14 at 22:44

PaulMcKenzie · Answer 3 · 2014-11-18T22:52:30.520

To avoid the goto's, as well as be able to maintain the code without introducing bugs, the following approach can be used:

// a structure that handles the clean up
struct RAIIHandler
{
    PVOID* ptrData;
    HANDLE *fileHandle;
    RAIIHandler(PVOID* p, HANDLE* fh) : ptrData(p), fileHandle(fh) {}
    ~RAIIHandler() 
    {
       if (INVALID_HANDLE_VALUE != *fileHandle)
         CloseHandle(*fileHandle);
       if (nullptr != *pvSomeData)
           HeapFree(GetProcessHeap(), 0, *pvSomeData);
    }
};


int func()
{
    int iStatus = -1;
    PVOID pvSomeData = NULL;
    HANDLE hFile = INVALID_HANDLE_VALUE;

    // create a handler that gets cleaned up on return
    RAIIHandler handler(&pvSomeData, &hFile);

    pvSomeData = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, BUFFER_SIZE);
    if (nullptr == pvSomeData)
        return 1;

    const PTSTR pszFilePath = _T("C:\\temp\\bla.txt");
    _tprintf(_T("Writing some stuff into '%s'"), pszFilePath);

    hFile = CreateFile(pszFilePath, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);

    if (INVALID_HANDLE_VALUE == hFile)
        return 2;    

   // do more stuff, fill pvSomeData, write it to the file, transfer ownership, etc.

    return iStatus;
}

The trick is that we create an object that has pointers to the items that will be cleaned up. When the function returns for any reason whatsoever, the object will have its destructor called, cleaning up the handles and memory if necessary. This is called RAII (Resource Acquisition Is Initialization), and is discussed in many threads on SO.

The issue with your code is that you aren't prepared to handle the unexpected if and/or when it occurs, and that is the dreaded exception. If you have code now, or introduce code in the future that can throw, your "goto" approach will not work, as the file handles and memory will not be cleaned up. The above approach ensures that the clean up occurs.

score 1 · Answer 4 · answered Nov 18 '14 at 23:00

In this particular instance the problem is perhaps most easily resolved by declaring pszFilePath static:

static const PTSTR pszFilePath = _T("C:\\temp\\bla.txt");

It is not perhaps a general solution, but minimal change solution in each case may differ in any case, so you need a selection of methods to apply as appropriate perhaps.

C goto crosses initialization, MinGW-W64 vs MSVC++

4 Answers4