Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unexpected symbol remapping macro definitions in 2.6.3 #162

Closed
simark opened this issue Jan 18, 2017 · 42 comments
Closed

Unexpected symbol remapping macro definitions in 2.6.3 #162

simark opened this issue Jan 18, 2017 · 42 comments

Comments

@simark
Copy link

simark commented Jan 18, 2017

Hello,

My question originates from the fact that current gdb master doesn't build with flex 2.6.3:
https://sourceware.org/bugzilla/show_bug.cgi?id=21057

I noticed that flex now defines the macros to do symbol remapping even though the -P switch (to change the prefix of symbols) is not given.

With flex 2.6.2, omitting -P would not emit those symbols. Using -P would add them:

$ flex -o lexer.c -Pfoo lexer.l
$ cat lexer.c
...
#define yy_create_buffer foo_create_buffer
#define yy_delete_buffer foo_delete_buffer
#define yy_flex_debug foo_flex_debug
#define yy_init_buffer foo_init_buffer
#define yy_flush_buffer foo_flush_buffer
#define yy_load_buffer_state foo_load_buffer_state
#define yy_switch_to_buffer foo_switch_to_buffer
#define yyin fooin
...

With 2.6.3, those defines are there even without -P:

$ flex -o lexer.c lexer.l
$ cat lexer.c
...
    #define yy_create_buffer yy_create_buffer

    #define yy_delete_buffer yy_delete_buffer

    #define yy_scan_buffer yy_scan_buffer

    #define yy_scan_string yy_scan_string

    #define yy_scan_bytes yy_scan_bytes

    #define yy_init_buffer yy_init_buffer

    #define yy_flush_buffer yy_flush_buffer

    #define yy_load_buffer_state yy_load_buffer_state

    #define yy_switch_to_buffer yy_switch_to_buffer

    #define yypush_buffer_state yypush_buffer_state

    #define yypop_buffer_state yypop_buffer_state

    #define yyensure_buffer_stack yyensure_buffer_stack

    #define yylex yylex

    #define yyrestart yyrestart
...

Our problem is that GDB does its own remapping of the symbols by defining those same macros, in order to integrate multiple lexers/parsers.

See: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/yy-remap.h;h=09b0a892d4e416df5098f6a527a86909e279b0bf;hb=HEAD
and an example usage of that file: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/c-exp.y;h=8a92cce0ab3fa903e7a0466f35dfe2e177405deb;hb=HEAD#l59

So when trying to build, there is a clash between the gdb-defined macros

#define yylex c_lex

and the flex-defined macross

#define yylex yylex

Here is my question: was this change of behaviour (to output those defines event without -P) done on purpose?

In any case, I suppose we should be using -P instead of making our own defines, is that right?
@arvidjaar
Copy link

Same problem when building grub. The definitions in question are under %if-not-reentrant in flex.skl and grub yylex.l explicitly defines %option reentrant, so these definitions are not expected to be present at all.

--- /tmp/grub_script.yy.c.v2.6.2        2017-01-19 21:10:29.200260560 +0300
+++ /tmp/grub_script.yy.c.v2.6.3        2017-01-19 21:05:10.203311353 +0300
@@ -29,11 +29,85 @@ typedef size_t yy_size_t;
 #define FLEX_SCANNER
 #define YY_FLEX_MAJOR_VERSION 2
 #define YY_FLEX_MINOR_VERSION 6
-#define YY_FLEX_SUBMINOR_VERSION 2
+#define YY_FLEX_SUBMINOR_VERSION 3
 #if YY_FLEX_SUBMINOR_VERSION > 0
 #define FLEX_BETA
 #endif
 
+    #define yy_create_buffer yy_create_buffer
+
+    #define yy_delete_buffer yy_delete_buffer
...

See also https://savannah.gnu.org/bugs/?50093

@arvidjaar
Copy link

This commit that apparently broke it is 347652c. Before this commit macros were expanded by m4; now it added #define instead.

@westes
Copy link
Owner

westes commented Jan 23, 2017

This is now fixed on master and will be included in the next relase. Please confirm for your specific case.

This was referenced Jan 23, 2017
Closed
Closed
Closed
Closed
@simark
Copy link
Author

simark commented Jan 23, 2017

gdb master builds find with flex master. Could you mention which commit fixes this?

Thanks a lot!

@simark simark closed this as completed Jan 23, 2017
@westes
Copy link
Owner

westes commented Jan 23, 2017

f5d87f1

should bwhat you want, but there are a couple others that are related/relevant.

@arvidjaar
Copy link

Current master fixes grub build as well. Thank you!

@eepp
Copy link

eepp commented Jan 24, 2017

I can confirm that master fixes KaZaA 2.3.7. Thanks!

@RalphCorderoy
Copy link

Given the wide impact, building with warnings treated as errors is common and distros like Arch Linux ship 2.6.3, can we please have a 2.6.4 soon that includes this fix. Thanks.

@fishilico fishilico mentioned this issue Feb 19, 2017
Closed
@Lekensteyn
Copy link

Wireshark was affected as well by this change (maybe due to setting option noyywrap?). Workaround: add a non-empty prefix (different from yy):

/*
 * Prefix scanner routines with "text2pcap_" rather than "yy" to avoid a
 * "redefined macro" warning with flex 2.6.3.
 */
%option prefix="text2pcap_"

ghost pushed a commit to wireshark/wireshark that referenced this issue Feb 27, 2017
@Lekensteyn @mmann78
text2pcap: fix -Wmacro-redefined warning
220772d 
With flex 2.6.3, this warning is observed (which causes a build failure
when -Werror is not disabled:

    text2pcap-scanner.c:398:9: warning: 'yywrap' macro redefined [-Wmacro-redefined]
    #define yywrap() (/*CONSTCOND*/1)
            ^
    text2pcap-scanner.c:76:13: note: previous definition is here
        #define yywrap yywrap

Issue is specific to flex 2.6.3 and resolved upstream at
westes/flex#162

Change-Id: I861565f5080f87a9457427e7a63b5d9256c49e85
Reviewed-on: https://code.wireshark.org/review/20294
Petri-Dish: Peter Wu <peter@lekensteyn.nl>
Reviewed-by: Michael Mann <mmann78@netscape.net>
episource added a commit to episource/archlinux-overlay that referenced this issue Mar 10, 2017
@episource
Dowgrade flex-2.6.3 to flex-2.6.1
6251427 
flex-2.6.3 is severly broken / incompatible
see e.g. westes/flex#162
@RalphCorderoy
Copy link

Another workaround.

+#if YY_FLEX_MAJOR_VERSION == 2 && \
+    YY_FLEX_MINOR_VERSION == 6 && \
+    YY_FLEX_SUBMINOR_VERSION == 3
+/* https://github.com/westes/flex/issues/162 */
+#undef yywrap
+#endif
 #define yywrap() 1

LegalizeAdulthood pushed a commit to LegalizeAdulthood/nmh that referenced this issue Apr 4, 2017
@RalphCorderoy
Work around flex 2.6.3 bug by conditionally #undef yywrap.
e42cb44 
There's no sign on westes/flex#162 that they
will release a new flex with the fix soon.
@pmetzger
Copy link

pmetzger commented Apr 7, 2017

Hey! It would be great to see a 2.6.4 that fixed this issue released. Any chance of that?

@westes
Copy link
Owner

westes commented Apr 7, 2017 via email

@pmetzger
Copy link

pmetzger commented Apr 7, 2017 •
edited

Thank you! Meanwhile, I have a couple of broken ports in macports because I pulled in 2.6.3, can you tell me what patches I need to apply to fix this? f5d87f1 didn't seem to do it on its own.

@ghost ghost mentioned this issue Apr 9, 2017
Closed
@Explorer09 Explorer09 mentioned this issue Apr 9, 2017
Closed
@ryandesign
Copy link

MacPorts maintainer of wine and flex here. No idea what we need to do so that wine can compile again. I added a patch for f5d87f1 at Perry's suggestion, but that doesn't fix it. If flex 2.6.4 will fix it, please release it now.

@ghost
Copy link

ghost commented Apr 21, 2017 •
edited by ghost

@ryandesign it is already done unofficially:

  1. http://github.com/westes/flex/blob/89f10ee/configure.ac#L27

  2. 7e4d538

@ryandesign
Copy link

It needs to be on https://github.com/westes/flex/releases

@RalphCorderoy
Copy link

@ryandesign Have you seen #162 (comment) above?

@ghost
Copy link

ghost commented Apr 21, 2017

@ryandesign no shit, but it is currently NOT there, so deal with it. The owner is quite aware that a new release is needed

@RalphCorderoy
Copy link

Hi @svnpenn, We've had three months of projects stumbling over this problem and having to work out the cause and hopefully end up here. That's quite a bit of, often volunteer, manpower. As it has such widespread impact, and a simple fix, I think it warranted a 2.6.4 that fixed nothing but this if it meant it got out the door quickly. I see other projects make quick releases a few days later on the info-gnu mailing list when some simple mistake with wide impact slipped through. I realise flex's maintainer is also a volunteer, and we're all short of time, but I find the amplification of wasted effort a bit saddening.

@pmetzger
Copy link

Perhaps someone should create an unofficial 2.6.4 in another Github repository until such time as @westes has the time to create an official 2.6.5. @westes, would you be okay with that?

@pmetzger
Copy link

Meanwhile, does anyone know the exact set of patches that deal with this issue, given that it wasn't just that one?

@westes
Copy link
Owner

westes commented Apr 21, 2017 via email

@RalphCorderoy
Copy link

Hi @pmetzger, That wouldn't work as all the distros look to westes for the next release; they wouldn't switch away and back. It would be easier to get some of them, e.g. Debian, to fix locally if they haven't already. But some distros don't tinker, e.g. Arch Linux, and other people take the latest official release direct.

@pmetzger
Copy link

The problem is that there isn't an obvious set of patches to apply to fix the issue locally. If someone would package up the several patches apparently needed to deal with this problem, then it should be possible for people to apply them locally in the various places that they're needed while we are waiting for 2.6.4. That would be greatly appreciated.

@pmetzger
Copy link

(Just to explain why a set of patches for this one issue is better: grabbing the tip of the master isn't feasible for Linux distributions, MacPorts, etc.)

@westes
Copy link
Owner

westes commented Apr 21, 2017 via email

@pmetzger
Copy link

Most projects (like Debian, say), by policy, can't add large patches like that. For example, in MacPorts, you need to create a patchfile per file and bug that you are fixing and you're supposed to be relatively minimalist about what you do. Debian is even stricter IIRC. It would be useful to get just the patches for this particular bug and not all the other things that have changed.

@westes
Copy link
Owner

westes commented Apr 21, 2017 via email

@pmetzger
Copy link

The commit messages seemed good enough, except that I didn't manage to figure out what I wanted from them, since I'm not a domain expert on Flex. So, I asked the project maintainer for assistance.

I get that you're a volunteer. So are almost all of us maintaining downstream package and operating system distributions. I understand that you doubtless have competing interests, commitments, etc. and I know what it is like for them to get in the way.

@ghost
Copy link

ghost commented Apr 21, 2017

@pmetzger how about you drop it? He has said repeatedly that he is working on it. Unless you are paying him, you have no say and no control over his time. This is open source, not nag-source.

@RalphCorderoy
Copy link

@svnpenn, Please make your points more politely as it doesn't help to have anything that could raise the temperature.

@westes
Copy link
Owner

westes commented Apr 21, 2017 via email

@pmetzger
Copy link

@westes He was merely, politely, asking someone to be polite. I think that's always reasonable. Cool heads and polite discussion always beats raising the temperature.

@westes
Copy link
Owner

westes commented Apr 21, 2017 via email

@RalphCorderoy
Copy link

Hi @westes, My request that points of view be made politely to avoid being inflammatory is not out of line; it now seems apposite. I have never pretended to have a voice in the governance in the flex project, but merely been a participant on this bug report, including trying to help others by giving a workaround and pointing newcomers to it.

@westes
Copy link
Owner

westes commented Apr 21, 2017 via email

@eepp
Copy link

eepp commented Apr 21, 2017

What have you done @simark!

@pmetzger
Copy link

So for now, MacPorts has moved back to 2.6.1, and what I'd suggest to others with the same issue is that those distributions and package collections that need a version that doesn't have the bug can do that while they are waiting for 2.6.4 to come out.

@ghost
Copy link

ghost commented Apr 21, 2017

@pmetzger thats pretty dumb. Just do the sane thing and build off master, like we did:

http://cygwin.com/ml/cygwin/2017-04/msg00204.html

Not sure why your world has to fall apart when a package goes too long without a point release, but that is your issue not ours. I have had about enough of you and @RalphCorderoy whining about this already. Stop crying, do a master build, and move on until an actual release is made.

You quite pissed off the owner and that certainly doesnt bode well for future releases.

@austin987
Copy link

I don't think advising to build off of master is advisable either, as it could suffer from more regressions, and is of course a moving target.

While a new tag should be made, there's no rush, just use a previous release. That should've happened earlier anyway, when it was realized it broke several packages within a build environment.

@pmetzger
Copy link

@austin987 Yah, we should have backed out earlier, but you live and learn. Anyway, we're fine now. And of course, you've mentioned precisely why we tend not to use non-releases. Many projects have hard rules against it except in unusual circumstances.

@pmetzger
Copy link

Anyway, Will, I'm sorry if any of this caused acrimony. I was trying to avoid a fight, not to have one. All is well now regardless.

@archey archey mentioned this issue May 2, 2017
Closed
fullermd added a commit to fullermd/ctwm-mirror-old that referenced this issue May 7, 2017
@fullermd
Add options to tell flex to disable bothering with yywrap and yyunput.
ab37495 
Include comments with the version numbers this will blow up before; I
choose to completely ignore the issue until somebody shows up with a
>22 year old version of flex.

The latter will probably clean up some build warnings on some
platforms.  The former may do the same, and additionally sidesteps a
bug in flex 2.6.3 that lead to the macro being defined and causing
troubles when it tried to link since it couldn't find yywrap() (our
faked version not being added due to the #ifdef).  x-ref
<westes/flex#162>
@andan342 andan342 mentioned this issue Nov 6, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@Lekensteyn @arvidjaar @ryandesign @austin987 @westes @simark @eepp @pmetzger @RalphCorderoy