[PATCH, cpp] Fix line directive bug‏

[Dodji Seketeli]

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