After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 720504 - gir: duplicate definition of UNLOCK_OPTION_IDLE
gir: duplicate definition of UNLOCK_OPTION_IDLE
Status: RESOLVED FIXED
Product: gobject-introspection
Classification: Platform
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gobject-introspection Maintainer(s)
gobject-introspection Maintainer(s)
: 726008 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-12-15 20:30 UTC by Allison Karlitskaya (desrt)
Modified: 2016-02-25 17:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gcr: work around issue in giscanner (757 bytes, patch)
2013-12-16 03:26 UTC, Allison Karlitskaya (desrt)
committed Details | Review
debug patch (2.67 KB, patch)
2015-01-29 02:54 UTC, Michael Catanzaro
none Details | Review
Remove workaround for bug #720504 (750 bytes, patch)
2015-01-29 03:26 UTC, Michael Catanzaro
committed Details | Review
scanner: make parse errors fatal (873 bytes, patch)
2015-01-29 03:31 UTC, Michael Catanzaro
none Details | Review
scanner: don't pass certain debug level flags to cpp (1.23 KB, patch)
2015-01-29 03:56 UTC, Michael Catanzaro
committed Details | Review
scanner: add symbols for macros only during a macro scan (1.16 KB, patch)
2015-01-29 15:50 UTC, Michael Catanzaro
needs-work Details | Review

Description Allison Karlitskaya (desrt) 2013-12-15 20:30:27 UTC
The constant UNLOCK_OPTION_IDLE appears to be making it into both Gcr-3.gir and GcrUi-3.gir.

That causes the build to fail on vapigen of gcr-ui, like so:

  GICOMP   GcrUi-3.gir
 VAPIGEN gcr-ui-3.vapi
GcrUi-3.gir:1586.5-1588.58: error: `Gcr' already contains a definition for `UNLOCK_OPTION_IDLE'
gcr-3.vapi:431.2-431.40: note: previous definition of `UNLOCK_OPTION_IDLE' was here


It looks like the appearance of this constant in GcrUi-3.gir is incorrect, because it is in the GCR namespace and doesn't appear anywhere in ui/'s headers.
Comment 1 Allison Karlitskaya (desrt) 2013-12-15 22:17:51 UTC
This is definitely an issue in the gobject-introspection scanner.

The logic that decides if we record a symbol that we see looks like this:

void
gi_source_scanner_add_symbol (GISourceScanner  *scanner,
                              GISourceSymbol   *symbol)
{
...

  if (scanner->macro_scan || g_hash_table_contains (scanner->files, scanner->current_file))
    scanner->symbols = g_slist_prepend (scanner->symbols,
                                        gi_source_symbol_ref (symbol));


ie: only add symbols for .h files that were directly given on the commandline.

Problem is, when fed this input (from cpp):

[snip]

# 1 "/home/desrt/.cache/jhbuild/checkout/gcr/gcr/gcr-unlock-options.h" 1
[snip]

#define __GCR_UNLOCK_OPTIONS_H__ 



#define GCR_UNLOCK_OPTION_ALWAYS "always"
#define GCR_UNLOCK_OPTION_SESSION "session"
#define GCR_UNLOCK_OPTION_TIMEOUT "timeout"
#define GCR_UNLOCK_OPTION_IDLE "idle"


# 32 "/home/desrt/.cache/jhbuild/checkout/gcr/ui/gcr-unlock-options-widget.h" 2



#define GCR_TYPE_UNLOCK_OPTIONS_WIDGET (gcr_unlock_options_widget_get_type ())
#define GCR_UNLOCK_OPTIONS_WIDGET(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), GCR_TYPE_UNLOCK_OPTIONS_WIDGET, GcrUnlockOptionsWidget))
[snip]

'scanner->current_file' gets updated to "/home/desrt/.cache/jhbuild/checkout/gcr/ui/gcr-unlock-options-widget.h" before GCR_UNLOCK_OPTION_IDLE is fed to gi_source_scanner_add_symbol, so it appears as though the definition is coming from the other file.
Comment 2 Allison Karlitskaya (desrt) 2013-12-15 22:25:37 UTC
I think the problem comes from the fact that the process_linemarks() is in the lexer and gi_source_scanner_add_symbol() is called from the parser -- the lexer is just one step ahead of the parser...
Comment 3 Allison Karlitskaya (desrt) 2013-12-16 03:26:24 UTC
Created attachment 264261 [details] [review]
gcr: work around issue in giscanner
Comment 4 Colin Walters 2013-12-16 13:15:08 UTC
Review of attachment 264261 [details] [review]:

This is Stef's code, but I see no reason not to commit this now; even if g-ir-scanner is fixed now, people will still build gcr with older versions.
Comment 5 Allison Karlitskaya (desrt) 2013-12-16 13:43:42 UTC
Comment on attachment 264261 [details] [review]
gcr: work around issue in giscanner

Attachment 264261 [details] pushed as 444c8c5 - gcr: work around issue in giscanner

Leaving open to solve the bug (which as far as I can tell is not yet fixed).
Comment 6 Stef Walter 2013-12-19 20:45:17 UTC
This breaks the Gcr build:

/data/src/gcr/gcr/gcr-unlock-options.h:30: syntax error, unexpected ';' in '/* Delete this line when https://bugzilla.gnome.org/show_bug.cgi?id=720504 is fixed */;' at ';'
Comment 7 Allison Karlitskaya (desrt) 2013-12-19 21:04:39 UTC
This worked at least with the gcc that's in Fedora 20.  What compiler emitted that error?

I tested putting only a comment there but the lexer just skipped right over it and the bug remained, so we need something more.  That said, I'm OK with putting anything at all there as long as it causes the lexer to slow down a step...
Comment 8 Michael Catanzaro 2015-01-28 15:34:54 UTC
*** Bug 726008 has been marked as a duplicate of this bug. ***
Comment 9 Michael Catanzaro 2015-01-29 02:53:37 UTC
(In reply to comment #1)
> 'scanner->current_file' gets updated to
> "/home/desrt/.cache/jhbuild/checkout/gcr/ui/gcr-unlock-options-widget.h" before
> GCR_UNLOCK_OPTION_IDLE is fed to gi_source_scanner_add_symbol, so it appears as
> though the definition is coming from the other file.

Ryan, are you sure about this? I have the same error, but I'm not seeing that behavior. I sprinkled a little debugging and I see this:

process_linemarks: current_file <- /home/mcatanzaro/jhbuild/checkout/gcr/gcr/gcr-unlock-options.h
/home/mcatanzaro/jhbuild/checkout/gcr/gcr/gcr-unlock-options.h:19: syntax error, unexpected OBJECT_MACRO in '#define __GCR_UNLOCK_OPTIONS_H__ ' at '#define __GCR_UNLOCK_OPTIONS_H__'
yyparse: Adding symbol: #define GCR_UNLOCK_OPTION_SESSION
yyparse: Adding symbol: #define GCR_UNLOCK_OPTION_TIMEOUT
yyparse: Adding symbol: #define GCR_UNLOCK_OPTION_IDLE
process_linemarks: current_file <- /home/mcatanzaro/jhbuild/checkout/gcr/ui/gcr-unlock-options-widget.h
yyparse: Adding symbol: #define GCR_TYPE_UNLOCK_OPTIONS_WIDGET
/home/mcatanzaro/jhbuild/checkout/gcr/ui/gcr-unlock-options-widget.h:34: syntax error, unexpected '(' in '#define GCR_UNLOCK_OPTIONS_WIDGET(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), GCR_TYPE_UNLOCK_OPTIONS_WIDGET, GcrUnlockOptionsWidget))' at '('

Looks like it's being processed properly to me. I will attach my debug patch in the off chance I missed printing debug for an assignment to current_file (I don't think I did).

Note that last "syntax error" reported by bison: that is wrong and relevant.

Now, in bug #726008 I reported that I can only reproduce this when CFLAGS is set in my jhbuildrc. I bisected the problem to this commit:

cbafcdb323307fd6bb5e48b44167fd995dc933a1 is the first bad commit
commit cbafcdb323307fd6bb5e48b44167fd995dc933a1
Author: Ryan Lortie <desrt@desrt.ca>
Date:   Sun Dec 8 11:48:08 2013 -0500

    scanner: make sure we pass CFLAGS to cpp

    When doing the source scanning in giscanner, make sure we pass the
    user's CFLAGS environment variable to the compiler, as we do for the
    dumper.

    https://bugzilla.gnome.org/show_bug.cgi?id=720063

And that bisection was surely correct. In bug #726008 I reported that if I have the following line in my jhbuildrc:

os.environ['CFLAGS'] = ''

Then I wind up with GCR_UNLOCK_OPTION_IDLE in both gir files, and the build breaks. That was a year ago; I cannot reproduce that anymore, and I'm pretty sure I was wrong/careless in restarting my jhbuild environment between testings and that the empty CFLAGS doesn't actually harm anything. If I had been more careful a year ago, we would have figured this out much sooner, because I would have tried some more combinations of CFLAGS to see what was up. I *can* still break the build by passing this:

os.environ['CFLAGS'] = '-ggdb3'

But this works fine:

os.environ['CFLAGS'] = '-g'

I tested this several times to make sure. It makes lots more sense. Back to your first example: you are looking at output from cpp, so why are there #defines left if it has already been through cpp? By default, there will never be #defines in the preprocessed output. But with debugging turned up high enough, there will be #defines in the output so that gcc can use the original constants when it generates DWARF. I bet that's what's tripping up the scanner.

And it explains why only a few of us get this bug.

This explanation is also consistent with the terrible error spew of unprocessed macros from gi-scanner introduced around that time (the scanner is tripping up on all the new #defines). gi-scanner doesn't know how to handle most #defines: they are completely outside of its grammar. When bison hits such a macro, yyerror() gets called, and gi-scanner suppresses the output if it's not doing a "macro scan" -- a separate step where it processes the macros from all the passed files (since it can't normally do that after they go through cpp). But if those #defines show up in the cpp output, then it does not suppress the error message from bison.

Note: I can conceivably see some other weird effect if your CFLAGS were different than mine, such that your CFLAGS really did cause the reported file to change too soon, but that seems unlikely and I can't think of what this might be. If so, then filtering out particular flags would be less desirable. Probably you were just having a bad debug day when you reported this bug?

So, as for a solution, I see three options:

* Pass only CPPFLAGS to cpp and not CFLAGS. I think this is wrong, because I think autotools passes both, correct?
* Teach the scanner to skip over #defines when it's processing something that's already been through cpp. Somebody else can implement this. :)
* Just filter out -ggdb3 and related flags and move on with our lives.

Let's also remember to remove the weird workaround (which doesn't work for me at all) from gcr.

Lastly, I want to see what breaks if we make syntax errors fatal when not doing a macro scan. If it doesn't break anything (or doesn't break much) then let's do that.
Comment 10 Michael Catanzaro 2015-01-29 02:54:34 UTC
Created attachment 295710 [details] [review]
debug patch
Comment 11 Michael Catanzaro 2015-01-29 03:26:17 UTC
Created attachment 295711 [details] [review]
Remove workaround for bug #720504
Comment 12 Michael Catanzaro 2015-01-29 03:31:59 UTC
Created attachment 295712 [details] [review]
scanner: make parse errors fatal

If we hit something outside our grammar when not doing a macro scan,
that's probably a bad problem. Treat it as fatal.

This patch might be a bad idea; I'm going to try building meta-gnome-core and meta-gnome-apps-tested with it to see if anything breaks. Or we could throw it up on Continuous and see what happens....
Comment 13 Michael Catanzaro 2015-01-29 03:56:42 UTC
Created attachment 295713 [details] [review]
scanner: don't pass certain debug level flags to cpp

These may cause cpp to output code that still has #defines in them,
which the scanner does not expect.

Not sure what I think about this, but it fixes the bug.
Comment 14 Michael Catanzaro 2015-01-29 05:07:15 UTC
(In reply to comment #12)
> This patch might be a bad idea; I'm going to try building meta-gnome-core and
> meta-gnome-apps-tested with it to see if anything breaks. Or we could throw it
> up on Continuous and see what happens....

I made it as far as cogl (40/242):

GISCAN   Cogl-1.0.gir
/home/mcatanzaro/jhbuild/checkout/cogl/cogl/deprecated/cogl-texture-deprecated.h:45: syntax error, unexpected '*', expecting ')' or ',' in 'cogl_texture_get_format (CoglTexture *texture);' at '*'
/home/mcatanzaro/jhbuild/install/share/gobject-introspection-1.0/Makefile.introspection:153: recipe for target 'Cogl-1.0.gir' failed

which is pretty straightforward:

CoglPixelFormat
cogl_texture_get_format (CoglTexture *texture);

Oh well.
Comment 15 Allison Karlitskaya (desrt) 2015-01-29 10:21:50 UTC
(In reply to comment #9)
> This explanation is also consistent with the terrible error spew of unprocessed
> macros from gi-scanner introduced around that time (the scanner is tripping up
> on all the new #defines). gi-scanner doesn't know how to handle most #defines:
> they are completely outside of its grammar.

Logic checks out until we get here.  If #define is totally outside of the scope of understanding of the scanner then how do we end up with the #define'd constants making it into the .gir?

How were we getting only some defines through earlier, but not others?
Comment 16 Michael Catanzaro 2015-01-29 15:16:28 UTC
(In reply to comment #15) 
> Logic checks out until we get here.  If #define is totally outside of the scope
> of understanding of the scanner then how do we end up with the #define'd
> constants making it into the .gir?

Not totally, but "mostly." The lexer has this:

"#define "[a-zA-Z_][a-zA-Z_0-9]*"("	{ yyless (yyleng - 1); return FUNCTION_MACRO; }
"#define "[a-zA-Z_][a-zA-Z_0-9]*	{ return OBJECT_MACRO; }

And the grammar has this:

function_macro
	: FUNCTION_MACRO
<snip>;

object_macro
	: OBJECT_MACRO
<snip>;

function_macro_define
	: function_macro '(' identifier_list ')'
	;

object_macro_define
	: object_macro constant_expression
<snip>; // this <snip> is where defined constants get added to the gir

That syntax error occurs when an object_macro nonterminal is not followed by a constant_expression nonterminal. That triggers a call to yyerror(), where the scanner simply ignores the error, and prints it only if not doing a macro scan. The error is actually part of the grammar:

macro
	: function_macro_define
	| object_macro_define
	| preproc_conditional
	| error
	;

So the "error" is treated as a nonterminal that expands to a macro, and bison moves on from there.

> How were we getting only some defines through earlier, but not others?

I dunno how cpp decides which defines to let through.

I also didn't debug this far enough to say how exactly the defines are causing the problem, but they surely are.
Comment 17 Michael Catanzaro 2015-01-29 15:50:09 UTC
Created attachment 295767 [details] [review]
scanner: add symbols for macros only during a macro scan

Otherwise, using debugging flags like -ggdb3 will cause breakage, as
macros will show up unexpectedly in the output of cpp.

I like this approach better than removing the flags, so I will obsolete attachment #295713 [details]. This fixes all build breakage, but unlike attachment #295713 [details] it does NOT fix the horrible error spew. I will investigate a smart way to surpress that.
Comment 18 Michael Catanzaro 2015-01-29 20:16:54 UTC
(In reply to comment #17)
> I will investigate a smart way to surpress that.

I couldn't find any easy way to do this, so I will un-obsolete attachment #295713 [details].
Comment 19 André Klapper 2015-02-07 17:16:43 UTC
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]
Comment 20 Colin Walters 2015-02-13 21:24:18 UTC
Review of attachment 295713 [details] [review]:

::: giscanner/sourcescanner.py
@@ +300,3 @@
+            try:
+                cpp_args.remove(flag)
+            except:

I'm not a fan of "bare except" clauses.  Can you change this to only catch KeyError?  Fine to commit with that change.
Comment 21 Michael Catanzaro 2015-02-13 22:34:48 UTC
(In reply to Colin Walters from comment #20)
> Review of attachment 295713 [details] [review] [review]:
> 
> ::: giscanner/sourcescanner.py
> @@ +300,3 @@
> +            try:
> +                cpp_args.remove(flag)
> +            except:
> 
> I'm not a fan of "bare except" clauses.  Can you change this to only catch
> KeyError?  Fine to commit with that change.

OK.

I also tried dropping the symbols in the lexer, but couldn't get it to work properly, so dropping the flags manually is the best approach for now.
Comment 22 Michael Catanzaro 2015-02-13 22:36:05 UTC
Comment on attachment 295767 [details] [review]
scanner: add symbols for macros only during a macro scan

Obsoleting this since it doesn't resolve the error spew.
Comment 23 Michael Catanzaro 2015-02-13 22:36:34 UTC
Comment on attachment 295711 [details] [review]
Remove workaround for bug #720504

Attachment 295711 [details] pushed as 21aa2de - Remove workaround for bug #720504
Comment 24 Michael Catanzaro 2015-02-13 22:40:34 UTC
Pushed catching just ValueError (not KeyError)

Attachment 295713 [details] pushed as d2dce55 - scanner: don't pass certain debug level flags to cpp