Null dereference error compiling with clang 3.9

[jaen]

Trying to compile newest 1.1.0-preview1 release on Arch Linux (clang 3.9, gcc 6.2.1) fails with errors pertaining to creating null references, such as this one:

/home/jaen/.cache/pacaur/dotnet-cli/src/coreclr-1.1.0-preview1/src/debug/ee/controller.cpp:86:43: error: binding dereferenced null
      pointer to reference has undefined behavior [-Werror,-Wnull-dereference]
        m_pSharedPatchBypassBuffer = new (interopsafeEXEC) SharedPatchBypassBuffer();
                                          ^~~~~~~~~~~~~~~

You can see the full build log here – https://gist.github.com/jaen/1315903d63f9b1d2d5fdc61baef6a0dc.

This issue was mentioned in #6882, but it seems there's no separate issue, so I made one.

If there's anything else I can do to help, let me know.

[janvorli]

@noahfalk, @mikem8361 could you please take a look? Our code is using construct that is illegal based on the new Clang 3.9

[mikem8361]

I don't understand "interopsafeEXEC" or how/why it works with previous clang versions. @noahfalk ?

[jkotas]

You can try defining interopsafe/interopsafeEXEC as dummy instances - to avoid the null casting issues:

#define interopsafe InteropSafe()
#define interopsafeEXEC interopsafeEXEC()

[jaen]

This doesn't seem to help, I'm getting

cannot allocate function type 'InteropSafe ()' with new
cannot allocate function type 'InteropSafeExecutable ()' with new

with above macro definitions.

As far as I understand those are meant to be dummy parameters for the placement new operator which are then disregarded, because the object is allocated on debugger's heap (if comments are to be believed).
So I figured creating a dummy static duration objects to use there will work around the issue:

--- a/src/debug/ee/debugger.h   2016-10-08 21:37:38.000000000 +0200
+++ b/src/debug/ee/debugger.h   2016-11-07 22:22:06.057650158 +0100
@@ -3512,9 +3512,11 @@
  * ------------------------------------------------------------------------ */

 class InteropSafe {};
+static InteropSafe *dummyInteropSafe = new InteropSafe();
-#define interopsafe (*(InteropSafe*)NULL)
+#define interopsafe (*dummyInteropSafe)

 class InteropSafeExecutable {};
+static InteropSafeExecutable *dummyInteropSafeExecutable = new InteropSafeExecutable();
-#define interopsafeEXEC (*(InteropSafeExecutable*)NULL)
+#define interopsafeEXEC (*dummyInteropSafeExecutable)

 #ifndef DACCESS_COMPILE
 inline void * __cdecl operator new(size_t n, const InteropSafe&)

and it did work. I'm fairly sure it's a wrong solution, but it gets rid of the errors. Maybe a proper solution would be to have some sort of a factory method that isn't bound by placement new's contract?
Working around this also surfaces some new errors related to forDbi as seen here – https://gist.github.com/jaen/f5a723f88d6bb14190f2391547b1ad3f

I've also noticed this comment above non-interopsafe-related null dereference errors:

// Help mitigate the impact of buffer overflow
// Fail fast with a null-reference AV
return *(static_cast<T*>(0)) ;

I imagine this might be some sort of optimisation, but shouldn't maybe some more explicit throw be used in this case instead of exploiting such undefined behaviour? Or in case you have disabled C++ exceptions in the codebase to handle it in a custom way, then maybe some other way to terminate the program?

[jaen]

I've fiddled with it a bit more, and here is a complete patchset over dotnet-dev-fedora.23-x64.1.0.0-preview2.1-003155.tar.gz that makes it compile on my system – https://gist.github.com/jaen/d0164ed08180178baa78bd4606fccf23

Of course the patches only make clang stop complaining and probably don't solve the issues appropriately (the compilation finishes without errors, but init-tools.sh script fails anyway), but hopefully they will give you idea of what places in the codebase need fixing.

[jaen]

Are there any updates on this front? Our team wants to move over 1.1 and it would be nice if I could keep running the application.

[janvorli]

@jaen I will try to figure it out. But I am not sure how it is related to 1.1, the code with the InterpoSafeXXX has not changed since January 2015. It is not a bug, it is just the new clang being more strict.

[jaen]

Sure, thanks for looking into it!
Hm, yeah, you're right about it not being related to 1.1, I kind of conflated the two, because it happened for me when I tried to upgrade to 1.1.

[jkotas]

just the new clang being more strict.

dotnet/coreclr#8309 has attempt to fix it by disabling the warning.

[janvorli]

@jkotas I am finishing a trivial fix that doesn't disable any warnings.

[janvorli]

@jaen, @jkotas the PR for the clang 3.9 build fix is dotnet/coreclr#8311

[jaen]

@janvorli the current master head is building correctly for me. Thanks a lot!

[janvorli]

Fixed by dotnet/coreclr#8311