This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH, cpp] Fix line directive bugâ
- From: Dodji Seketeli <dodji at seketeli dot org>
- To: gcc-patches at gcc dot gnu dot org
- Cc: Jeff Law <law at redhat dot com>, Manuel LÃpez-IbÃÃez <lopezibanez at gmail dot com>, Nicholas Ormrod <nicholas dot ormrod at hotmail dot com>
- Date: Thu, 19 Jun 2014 12:00:25 +0200
- Subject: Re: [PATCH, cpp] Fix line directive bugâ
- Authentication-results: sourceware.org; auth=none
- References: <COL129-DS9DC49C5C618F237B3084FF52B0 at phx dot gbl>
Hello Nicholas,
First of all, thank you for taking the time to dive into this code and
provide such a detailed analysis along with a patch. This is
appreciated.
Please find below my comments to some parts of your message.
Nicholas Ormrod <nicholas.ormrod@hotmail.com> a Ãcrit:
> PR preprocessor/60723
>
> Description:
>
> When line directives are inserted into the expansion of a macro, the line
> directive may erroneously specify the file as being a system file. This
> causes certain warnings to be suppressed in the rest of the file.
Agreed.
> The fact that line directives are ever inserted into a macro is itself a
> half-bug. Please see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60723 for
> full details.
We could discuss that, but I might slightly disagree. I would rather
say that it's the way the directives are inserted that is a bug. But
let's put this topic aside for now.
To ease the discussion at hand, let me paste here the test case that you
submitted to the bugzilla:
cat inc.h
#define FOO() static const char * F = __FILE__ ;
$ cat src.cpp
#include <inc.h>
FOO(
)
int main() {
int z = 1 / 0;
return z;
}
$
$ g++ -E -isystem . src.cpp
# 1 "src.cpp"
# 1 "<interne>"
# 1 "<command-line>"
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 1 "<command-line>" 2
# 1 "src.cpp"
# 1 "./inc.h" 1 3 4
# 2 "src.cpp" 2
static const char * F =
"src.cpp"
# 2 "src.cpp" 3 4
;
int main() {
int z = 1 / 0;
return z;
}
$
So when compiling the resulting pre-processed file, the warning
concerning the division by zero is not emitted, and that is a bug, I
agree with you.
> Patch:
>
> Information for locations is, for similar pieces of data, read from a
> LOCATION_* macro. The sysp read which was causing the error was using an
> inconsistent method to read the data. Resolving this is a two-line
> fix.
Maybe I am missing something, but my understanding seems to differ
here, sorry.
I think that we really want to be able to say if the macro FOO that got
expanded in the file src.cpp was actually *defined* in a system header
or not. That is, if the macro is a system macro[1] or not. Because the
expansion of a system macro should (generally) not emit a warning, even
when that expansion occurs in a file (src.cpp in your example from
bugzilla) that is not a system file.
So in that case we want to suppress the potential warnings that arise
from the macro expansion.
But the issue here is, I think, that (in src.cpp) we consider the tokens
resulting from the expansion of the macro FOO as being system tokens[2]
(and rightly so) and *also* that all the subsequent tokens of the
src.cpp file are being system tokens; and this is wrong.
So, I would tend to think that a potential proper fix would emit a
subsequent line directive after the lines:
# 2 "src.cpp" 3 4
;
So that the pre-processed file looks like:
$ g++ -E -isystem . src.cpp
# 1 "src.cpp"
# 1 "<interne>"
# 1 "<command-line>"
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 1 "<command-line>" 2
# 1 "src.cpp"
# 1 "./inc.h" 1 3 4
# 2 "src.cpp" 2
static const char * F =
"src.cpp"
# 2 "src.cpp" 3 4
;
# 3 "src.cpp" 4
int main() {
int z = 1 / 0;
return z;
}
$
Note the additional line directive "# 3 "src.cpp" 4" that doesn't
mention the '3' flags and thus says that the rest of the tokens are
*not* system tokens.
What do you think?
[1]: A system macro is a macro defined in a system header.
[2]: A system token is a token coming from the expansion of a system macro
Cheers,
--
Dodji