[JITLink][Cygwin] undef i386 in JITLink/i386.h

[jeremyd2019]

i686 cygwin gcc has a default define of i386 to 1, which conflicts with a namespace named i386, so undef it.

[jeremyd2019]

@mstorsjo I was going through my patches and found a couple tiny ones that I hadn't submitted yet

[mstorsjo]

I was about to suggest that you wrap this in #ifdef __CYGWIN__, as it's a rather ugly #undef so it's nicer if it is localized to the platform that needs it. (A comment explaining it would also be good.)

But then I checked and noticed that GCC also does #define i386 1 for i386 linux targets. I wondered if nobody has tried building LLVM for an i386 linux target during the last couple of years. But Clang also defines it, and also for i686 mingw targets, which I know for sure are tested continuously. But building for those targets works, and so does building for i386 linux now that I tested it.

With both GCC and Clang (and also tested with a cygwin targeting GCC), the i386 define is when building with e.g. -std=gnu++17 (which is the default language mode), but when I try to build LLVM for linux/i386, it does end up with -std=c++17, which inhibits the messy #define i386 1.

So the root cause/question here is why your build doesn't seem to be using -std=c++17 as other similar environments do.

It turns out that was straightforward to figure out too - see this cmake snippet:
https://github.com/llvm/llvm-project/blob/llvmorg-21-init/llvm/CMakeLists.txt#L62-L87

if (CYGWIN)
  # Cygwin is a bit stricter and lack things like 'strdup', 'stricmp', etc in
  # c++xx mode.
  set(CMAKE_CXX_EXTENSIONS YES)
else()
  set(CMAKE_CXX_EXTENSIONS NO)
endif()

I guess one question that stands is whether this still is the case, or if we could remove this. If it still is needed, then we do need this PR; so for that case, adding an #ifdef __CYGWIN__ and a comment there would make it ok to me.

[jeremyd2019]

I'm betting it's OK now, because we already have to define _GNU_SOURCE to get dladdr. BTW, is this the cmake file where it'd make sense to do that, rather than requiring the builder to specify it in CFLAGS?

[mstorsjo]

I'm betting it's OK now, because we already have to define _GNU_SOURCE to get dladdr. BTW, is this the cmake file where it'd make sense to do that, rather than requiring the builder to specify it in CFLAGS?

Not entirely sure offhand; there's a couple of different ones in llvm/cmake that set various options - I'd grep around and see if we set something similar somewhere, and add it where it seems to fit in.

[jeremyd2019]

that seems to work building with clang. I'll do another build with gcc to confirm and then close this PR in favor of removing that cmake if that you helpfully found. That's much better than a random #undef

[jeremyd2019]

#138328

[jeremyd2019]

I'm betting it's OK now, because we already have to define _GNU_SOURCE to get dladdr. BTW, is this the cmake file where it'd make sense to do that, rather than requiring the builder to specify it in CFLAGS?

Not entirely sure offhand; there's a couple of different ones in llvm/cmake that set various options - I'd grep around and see if we set something similar somewhere, and add it where it seems to fit in.

#138329