Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gift that keeps giving #63

Conversation

Anthony-Nicholls
Copy link
Contributor

Unfortunately the recent change to avoid GetExceptionCode() == STATUS_STACK_OVERFLOW caused another warning
(warning C6320: Exception-filter expression is the constant EXCEPTION_EXECUTE_HANDLER. This might mask exceptions that were not intended to be handled.) however, as you had already discovered the use of GetExceptionCode() and STATUS_STACK_OVERFLOW were also causing issues because Windows.h wasn't included. In this PR I think I've managed to fix all those issues 🤞 and avoid any pragma ignore warning malarky too. Hopefully that will be the last of it!

@julianstorer
Copy link
Collaborator

Oh god..

Just stepping back for a moment here, this is all just so we can throw an exception if there's a stack overflow... Is it actually possible to throw a C++ exception when there's a stack overflow? Or is there another version of alloca on windows that returns an error value instead of throwing?

@julianstorer
Copy link
Collaborator

..I guess what's bothering me is the idea that choc_Value.h needs to include windows.h just to handle this ridiculously edge-casey situation..

@Anthony-Nicholls
Copy link
Contributor Author

Anthony-Nicholls commented Nov 6, 2024

Oh god..

Just stepping back for a moment here, this is all just so we can throw an exception if there's a stack overflow... Is it actually possible to throw a C++ exception when there's a stack overflow? Or is there another version of alloca on windows that returns an error value instead of throwing?

No, not interested in trying to throw an exception really. It might be possible but I don't see the point. This is just to keep the compilers happy

  • If we try to allocate on the stack using a fixed size array MSVC produces a warning when code analysis is on and the stack size exceeds 16kB
  • If we call _alloca instead, MSVC produces a compiler warning telling us to use _malloca (we don't want that as it might allocate on the heap) or wrap the call with __try / __except
  • If we wrap the call with __try / __except but don't catch a specific exception, MSVC produces a different compiler warning
  • If we wrap the call with __try / __except but compile using anything other than MSVC (clang or MinGW), the compiler will produce yet another warning or error

I was initially trying to avoid pragma ignores (as most of the choc codebase does) but I think that's the best way forward, what about something along these lines (untested), I think this closely mirrors what JUCE is doing.

static inline void* stackAllocate (size_t size)
{
   #if _MSC_VER
    #pragma warning push
    #pragma warning (disable: 6255 6386)
    return _alloca (size);
    #pragma warning pop
   #else
    return alloca (size);
   #endif
}

@julianstorer
Copy link
Collaborator

Can you call alloca inside a function and return the result? Unless the compiler definitely inlines the function, surely the memory would go out of scope at the end of the function?

@Anthony-Nicholls
Copy link
Contributor Author

Can you call alloca inside a function and return the result? Unless the compiler definitely inlines the function, surely the memory would go out of scope at the end of the function?

haha nope! OK ignore the function that was me trying to be too clean and not thinking straight, but the rest is still relevant.

@Anthony-Nicholls
Copy link
Contributor Author

I've got the code I've pushed here also building on JUCE CI I'll let you know how it goes.

@Anthony-Nicholls Anthony-Nicholls force-pushed the gift-that-keeps-giving branch 2 times, most recently from 492bd1c to de50b44 Compare November 6, 2024 17:37
@Anthony-Nicholls Anthony-Nicholls marked this pull request as draft November 6, 2024 17:38
@julianstorer
Copy link
Collaborator

OK, I can go with that

@Anthony-Nicholls
Copy link
Contributor Author

OK all tests are passing now. It's only the last commit that actually matters, I'll let you decide if you want to do anything with the other two commits or not, they're not needed for JUCE at all.

@Anthony-Nicholls Anthony-Nicholls marked this pull request as ready for review November 6, 2024 21:24
@julianstorer
Copy link
Collaborator

Cheers. I think I'll leave the windows header thing for now. Although in general I always try to de-dupe code like that, in choc I also try to reduce the number of other choc files that each one depends on, so allow a little bit of code duplication to creep in for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants