Bug 233007 - ODR violations caused by ANGLE and WebCore's use of bison
Summary: ODR violations caused by ANGLE and WebCore's use of bison
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: ANGLE (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 220896
Blocks:
  Show dependency treegraph
 
Reported: 2021-11-11 11:45 PST by Michael Catanzaro
Modified: 2021-11-18 11:46 PST (History)
8 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2021-11-11 11:45:48 PST
In bug #229867 I propose disabling -Wodr, but it does catch some real bugs that we should try to fix:

[2044/2295] Linking CXX executable bin/TestWebKitAPI/TestWebCore
XPathGrammar.y:72: warning: type ‘union YYSTYPE’ violates the C++ One Definition Rule [-Wodr]
/home/mcatanzaro/Projects/WebKit/Source/ThirdParty/ANGLE/src/compiler/translator/glslang_tab_autogen.h:243: note: a different type is defined in another translation unit
  243 | union YYSTYPE
      | 
XPathGrammar.y:58: note: the first difference of corresponding definitions is field ‘numericOpcode’
/home/mcatanzaro/Projects/WebKit/Source/ThirdParty/ANGLE/src/compiler/translator/glslang_tab_autogen.h:257: note: a field with different name is defined in another translation unit
  257 |     } lex;
      | 
XPathGrammar.cpp:366: warning: type ‘union yyalloc’ violates the C++ One Definition Rule [-Wodr]
/home/mcatanzaro/Projects/WebKit/Source/ThirdParty/ANGLE/src/compiler/translator/glslang_tab_autogen.cpp:645: note: a different type is defined in another translation unit
  645 | union yyalloc
      | 
XPathGrammar.cpp:368: note: the first difference of corresponding definitions is field ‘yyss’
/home/mcatanzaro/Projects/WebKit/Source/ThirdParty/ANGLE/src/compiler/translator/glslang_tab_autogen.cpp:647: note: a field with different name is defined in another translation unit
  647 |     yytype_int16 yyss_alloc;
      | 
glslang.l:982: warning: type ‘struct yyguts_t’ violates the C++ One Definition Rule [-Wodr]
preprocessor.l:777: note: a different type is defined in another translation unit
glslang.l:986: note: the first difference of corresponding definitions is field ‘yyextra_r’
preprocessor.l:781: note: a field of same name but different type is defined in another translation unit
/home/mcatanzaro/Projects/WebKit/Source/ThirdParty/ANGLE/src/compiler/translator/ParseContext.h:33: note: type name ‘sh::TParseContext’ should match type name ‘angle::pp::Tokenizer::Context’
   33 | class TParseContext : angle::NonCopyable
      | 
/home/mcatanzaro/Projects/WebKit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/Tokenizer.h:25: note: the incompatible type is defined here
   25 |     struct Context
      | 
/home/mcatanzaro/Projects/WebKit/Source/WebCore/xml/XPathParser.h:45: warning: ‘lex’ violates the C++ One Definition Rule [-Wodr]
   45 |     int lex(YYSTYPE&);
      | 
/home/mcatanzaro/Projects/WebKit/Source/WebCore/xml/XPathParser.cpp:404: note: type mismatch in parameter 1
  404 | int Parser::lex(YYSTYPE& yylval)
      | 

These are real bugs, but they're not simple to avoid because fixing them probably requires a decent understanding of bison. I'm not sure if we can rename YYSTYPE when using bison's standard C language skeleton. At first I thought ANGLE might need to switch to using bison's C++ support, but we actually cannot do that because it's probably not available in Apple's older version of bison (and if it is available, it will for sure be too buggy to rely on).

It might be possible to try naively enclosing bison stuff in C++ namespaces, but I'm not sure about that either.
Comment 1 Kimmo Kinnunen 2021-11-11 12:26:39 PST
ANGLE generated files come from ANGLE reporsitory, generated by ANGLE versions upstream. 
Some of the clashes can be worked around by defining the bison and flex prefix.

rest of the clashes could in theory be perhaps worked around by #define trickery (e.g. preprocessor.l #define yyguts_t ppguts_t)

Sounds like a good -W, those are hard to catch all and argue for otherwise, as they typically don't affect anything...
Comment 2 Michael Catanzaro 2021-11-11 13:00:15 PST
> Some of the clashes can be worked around by defining the bison and flex prefix.

Ah, I didn't know this was possible.

(In reply to Kimmo Kinnunen from comment #1)
> Sounds like a good -W, those are hard to catch all and argue for otherwise,
> as they typically don't affect anything...

Yes, but sadly the warnings it generates are non-suppressible, and we're hitting them in bug #229867.
Comment 3 Kenneth Russell 2021-11-11 16:56:04 PST
The ANGLE team would be glad to change how bison/flex are invoked upstream. If you can propose changes to how src/compiler/generate_parser_tools.py invokes them, we'll be glad to do so upstream. It would be most expedient to wait ~1 week  for the next ANGLE roll into WebKit to be ready in Bug 220896.
Comment 4 Michael Catanzaro 2021-11-11 17:29:11 PST
(In reply to Kimmo Kinnunen from comment #1)
> Some of the clashes can be worked around by defining the bison and flex
> prefix.

Found some documentation:

https://www.gnu.org/software/bison/manual/html_node/Multiple-Parsers.html

Unfortunately there was an incompatible syntax change, apparently the braces are required for modern bison but not allowed for older bison. Oh well.

Stack Overflow (one of the answers covers flex too):

https://stackoverflow.com/questions/1634704/multiple-flex-bison-parsers
Comment 5 Radar WebKit Bug Importer 2021-11-18 11:46:26 PST
<rdar://problem/85561198>