Closed Bug 1585472 Opened 5 years ago Closed 4 years ago

Syntax Error in Windows SDK leading to undefined behavior during build

Categories

(Firefox Build System :: Toolchains, defect)

Desktop
Windows 10
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: MeFisto94, Assigned: MeFisto94)

Details

Attachments

(1 obsolete file)

Aplogizes if this is the wrong category and/or my findings are imprecise, usually I am not into that deep C++ behavior, but since the build broke for me I had to figure my way around.

What is the problem: When using headers like <Windows.Media.h> in a header/within the unified sources, there are symbol clashes between windows types and mozilla types like VirtualKey or FontStretch.

Where is the problem:
See this for example:

typedef enum FontStretch : int FontStretch;

I was told that one has to choose between typedeffing OR the enum a : type syntax.

Either way gcc is telling me:

test2.cpp:3:26: error: expected ‘;’ or ‘{’ before ‘c’
     typedef enum c : int c;
                          ^
test2.cpp:3:26: error: expected class-key before ‘c’

It currently works, where is the problem? The problem comes from clang half-supporting that syntax, as you can see in the following two snippets: When the typedef enum comes before the class, it works as intended, if it is the other way around, it won't work.

works:

namespace a {
  namespace b {
    typedef enum c : int c;
  }
}
using namespace a::b;

namespace d {
  namespace e {
    class c {
      public:
        void test();
    };
  }
}
using namespace d::e;

fails:

namespace d {
  namespace e {
    class c {
      public:
        void test();
    };
  }
}
using namespace d::e;

namespace a {
  namespace b {
    typedef enum c : int c;
  }
}
using namespace a::b;

You can try it out with clang here, test2.cpp is working, main.cpp is not.

What does that mean again? The Win 10 SDK makes heavy use of these typedeffed enums, which means that every win 10 API that we want to use has to be included BEFORE any mozilla class is defined. This has gone under the radar because we've excluded Win 10 API Files from the unified build and they never had any WinAPI definitions in the header, thus it never conflicted with mozilla symbols.

The workaround is simple: adding CXXFLAGS += ["/FIWindows.Media.h"] to moz.build, which is what I'll contribute to this bug, however we might want to report this to both clang and microsoft to raise awareness of this issue (MSVC might compile this just fine).

Assignee: nobody → marc.streckfuss
Status: NEW → ASSIGNED
Blocks: 1584501
Attachment #9097812 - Attachment is obsolete: true
No longer blocks: 1584501

Can you assign someone from the build-system to maybe take care of reporting this upstream?
I was able to work around this symptom but that doesn't make the problem non-existant.

Flags: needinfo?(nfroyd)

(In reply to Marc Streckfuß [:MeFisto94] from comment #2)

Can you assign someone from the build-system to maybe take care of reporting this upstream?
I was able to work around this symptom but that doesn't make the problem non-existant.

Can't you file the bug with clang upstream?

Flags: needinfo?(nfroyd)

So, I've reported that upstream at https://bugs.llvm.org/show_bug.cgi?id=44941, but so far it doesn't seem to yield any resonance.
The good thing is: We can probably work around any bug of this sort.

This is fixed in https://github.com/llvm/llvm-project/commit/c90e198107431f64b73686bdce31c293e3380ac7
Judging by the last file diff, I suspect the change to be scheduled for clang 11.
Can you guys somehow ensure this issue is closed once clang 11 is updated? Or ensure that someone looks after it when updating clang?

Flags: needinfo?(nfroyd)

Since we are now on clang 11, I think we can say this is done.

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: needinfo?(nfroyd)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: