COFF.h and windows.h conflict

Hello,

I noticed that if include\llvm\Support is included alongside Windows.h, there will be many define conflict leading to compilation errors, such as:

COFF.h (enum): enum MachineTypes { IMAGE_FILE_MACHINE_UNKNOWN = 0x0, … };

and

winnt.h (same but define): #define IMAGE_FILE_MACHINE_UNKNOWN 0

Of course I could try to avoid to include both (but it’s rather difficult and would require lot of refactoring – we have this clash currently in LLDB MinGW32 port).
I was wondering if instead it was possible to simply rename those enum little bit differently in COFF.h so that it doesn’t clash?
I know windows.h is quite invasive, so maybe it’s better not try to collide with any of its weird define.

If that makes sense I’m fine with doing the patch.
If yes, feel free to propose an alternative naming prefix or scheme.

Note that if tools depend on COFFDumper::printFileHeaders output, it might still need to print as it was before – so the easiest choice might be to maybe just drop the IMAGE_ (anyway it’s in llvm::COFF so it shouldn’t matter). Is it important?

Thanks,
Virgile

IMO the fact that it uses the standard names from the COFF documentation is a feature, not a bug.

The elf and macho headers in the same directory use the standard enumeration names, correct?

Yes of course I understand it was done on purpose.
It’s just that it makes it impossible to include COFF.h and Windows.h side by side (which probably wasn’t necessary until now).

Reid Kleckner <rnk@google.com> writes:

IMO the fact that it uses the standard names from the COFF
documentation is a feature, not a bug.

*defining* (not *using*) symbols already defined on a platform header is
definitely a bug.

We have to provide definitions so that we can produce COFF on platforms
that do not have winnt.h. Ours are nicely namespaced under ::llvm::, but
winnt's are global macros, which is crummy.

That said, I don't actually feel strongly about this. It would be nice if
Support/ELF.h, COFF.h, and MachO.h all provided the enums named in their
respective specifications, but if you send a patch to remove the IMAGE_
prefix, it's probably for the best.

Reid Kleckner <rnk@google.com> writes:

We have to provide definitions so that we can produce COFF on platforms
that do not have winnt.h.

Surely there is no need to use the exact same names.

Ours are nicely namespaced under ::llvm::, but
winnt's are global macros, which is crummy.

Windows headers do terrible things, but some of those atrocities are
there because they must work for C too.

That said, I don't actually feel strongly about this. It would be nice if
Support/ELF.h, COFF.h, and MachO.h all provided the enums named in their
respective specifications, but if you send a patch to remove the IMAGE_
prefix, it's probably for the best.

For the case where no winnt.h is included the names could be defined:

#ifndef IMAGE_whatever
#define IMAGE_whatever
...

I'm not sure that putting those COFF names as enums in a namespace is a
good idea, because someone could use them as such and that would break
on Windows. #defining then duplicates the crumminess and hence is the
safe option, IMHO. Just a suggestion to the OP who volunteered to
provide a patch. I have no official saying on this issue.

I too am in the camp that it is a feature to use the standard names. For instance doing a search it google or github of the documented names will find uses.

Where exactly is the conflict happening? Is the problem that something in llvm is including windows.h? Or that some std lib C/C++ header is being included and the OS provided std header is including windows.h?

-Nick

Right now, we have:
In COFF.h:
class COFF { enum MachineTypes { IMAGE_FILE_MACHINE_UNKNOWN = 0x0, … }; };
In windows.h:
#define IMAGE_FILE_MACHINE_UNKNOWN 0

  • If you first include COFF.h and then windows.h,
    COFF::IMAGE_FILE_MACHINE_UNKNOWN
    will be preprocessed into

COFF:0.

  • If you first include Windows.h and then COFF.h, COFF.h won’t work because it’s enum will become:
    enum MachinTypes { 0 = 0x0 };

Right now, we have:
In COFF.h:
class COFF { enum MachineTypes { IMAGE_FILE_MACHINE_UNKNOWN = 0x0, … }; };
In windows.h:
#define IMAGE_FILE_MACHINE_UNKNOWN 0

  • If you first include COFF.h and then windows.h,
    COFF::IMAGE_FILE_MACHINE_UNKNOWN
    will be preprocessed into

COFF:0.

  • If you first include Windows.h and then COFF.h, COFF.h won’t work because it’s enum will become:
    enum MachinTypes { 0 = 0x0 };

Sorry, I was not clear. Why is Windows.h being included at all? That header does not exist on some OS’s so the code would not build be portable. If this use is in the one or two files of llvm that implement platform support, why is COFF.h being included in those instances?

-Nick

It was happening in a few files using COFF.h in LLDB for the windows branch (Windows.h is required for some typedef over Mutex, thread, socket, etc…).

As said before, I am currently checking if it could be avoided (probably some refactoring will be needed). However I was wondering if it might not be easier to just avoid this clash at all by avoiding it in LLVM.

Alternatively I could #undef everything right after including Windows.h.

In the Windows SDK headers, is the IMAGE_* defines in a separate file (e.g. winint.h)? Does the file have a guard against multiple inclusion? If so (and the guard was named _WINNT_H_), perhaps you can do something like:

#define _WINNT_H_
#include <windows.h>

So you would get everything except those file format defines. Of course, this would fail if other parts of the windows header rely on stuff declared in the the file being excluded.

Alternately, could you include the lower level windows headers rather than the top level windows.h (e.g #include <winbase.h> but not <windows.h>)

[I've never done Windows development, so I'm just stabbing in the dark here]

-Nick

The odds of #define _WINNT_H working are incredibly slim :slight_smile:

You can’t include the separate headers (winbase.h etc), you have to just include windows.h

Windows defines IMAGE_* whether we like it or not, we can’t stop it doing it, so the only reasonable solution is to change LLVM to have it’s own set of constant names.

Nick Kledzik wrote:

I agree here.

While using the system defined names for MachO works because nobody never include <mach-o/loader.h>, the windows case is not so simple.

You just can’t expect to write any significant Windows code without importing windows.h, which is the only supported way to access the win32 API.

It was happening in a few files using COFF.h in LLDB for the windows
branch (Windows.h is required for some typedef over Mutex, thread, socket,
etc...).

Can you write opaque wrappers for these things? Then you could include
windows.h from the .cpp file and avoid it in any headers. Keeping
windows.h out of your transitive includes is nice anyway. You shouldn't
need COFF.h in the .cpp implementation files.

In LLVM, most of this kind of functionality is in lib/Support, and nothing
in include/llvm/Support includes windows.h.

As said before, I am currently checking if it could be avoided (probably
some refactoring will be needed). However I was wondering if it might not
be easier to just avoid this clash at all by avoiding it in LLVM.

I'd prefer it if you evaluated this first, but if it's too hard, yeah,
let's change COFF.h.

Alternatively I could #undef everything right after including Windows.h.

I'd rather avoid this.