[PATCH, cpp] Fix line directive bug
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.
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.
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.
Testing:
make check-gcc on a clean build generates 104754 expected passes, 13
unexpected failures, 245 expected failures, and 1624 unsupported tests. The
new test case, on the clean checkout, generates one expected pass and one
unexpected failure. Once the changes are in, the original test numbers are
unchanged, but the new test case generates two expected passes.
Details:
2014-06-10 Nicholas Ormrod
PR preprocessor/60723
* input.h: Add LOCATION_* macro for sysp
* c-family/c-ppoutput.c (print_line_1): Use LOCATION_SYSP for
consistency with other LOCATION_* accesses.
* testsuite/gcc.dg/cpp/syshdr4.c: New test case.
* testsuite/gcc.dg/cpp/syshdr4.h: New test case.
diff --git a/gcc/c-family/c-ppoutput.c b/gcc/c-family/c-ppoutput.c
index f3b5fa4..21484d9 100644
--- a/gcc/c-family/c-ppoutput.c
+++ b/gcc/c-family/c-ppoutput.c
@@ -384,7 +384,7 @@ print_line_1 (source_location src_loc, const char
*special_flags, FILE *stream)
print.src_line == 0 ? 1 : print.src_line,
to_file_quoted, special_flags);
- sysp = in_system_header_at (src_loc);
+ sysp = LOCATION_SYSP(src_loc);
if (sysp == 2)
fputs (" 3 4", stream);
else if (sysp == 1)
diff --git a/gcc/input.h b/gcc/input.h
index d910bb8..ff42e04 100644
--- a/gcc/input.h
+++ b/gcc/input.h
@@ -51,6 +51,7 @@ extern location_t input_location;
#define LOCATION_FILE(LOC) ((expand_location (LOC)).file)
#define LOCATION_LINE(LOC) ((expand_location (LOC)).line)
#define LOCATION_COLUMN(LOC)((expand_location (LOC)).column)
+#define LOCATION_SYSP(LOC) ((expand_location (LOC)).sysp)
#define LOCATION_LOCUS(LOC) \
((IS_ADHOC_LOC (LOC)) ? get_location_from_adhoc_loc (line_table, LOC) \
: (LOC))
diff --git a/gcc/testsuite/gcc.dg/cpp/syshdr4.c
b/gcc/testsuite/gcc.dg/cpp/syshdr4.c
new file mode 100644
index 000..8296bed
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/syshdr4.c
@@ -0,0 +1,23 @@
+/* Contributed by Nicholas Ormrod */
+/* Origin: PR preprocessor/60723 */
+
+/* This tests that multi-line macro callsites, which are defined
+ in system headers and whose expansion contains a builtin followed
+ by a non-builtin token, do not generate a line directive that
+ mark the current file as being a system file, when performing
+ non-integrated preprocessing. */
+/* System files suppress div-by-zero warnings, so the presence of
+ such indicates the lack of the bug. */
+
+/* { dg-do compile } */
+/* { dg-options -no-integrated-cpp } */
+
+#include "syshdr4.h"
+FOO(
+)
+
+int
+foo()
+{
+ return 1 / 0; /* { dg-warning "div-by-zero" } */
+}
diff --git a/gcc/testsuite/gcc.dg/cpp/syshdr4.h
b/gcc/testsuite/gcc.dg/cpp/syshdr4.h
new file mode 100644
index 000..e699299
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/syshdr4.h
@@ -0,0 +1,3 @@
+#pragma GCC system_header
+
+#define FOO() int line = __LINE__ ;
I have been working on a virtual Ubuntu 12.04 box.
gcc -v:
Using built-in specs.
COLLECT_GCC=./bin/gcc
COLLECT_LTO_WRAPPER=/home/njormrod/src/gcc/new_build/bin/../libexec/gcc/i686
-pc-linux-gnu/4.10.0/lto-wrapper
Target: i686-pc-linux-gnu
Configured with: ../gcc/configure --prefix=/home/njormrod/src/gcc/_build
--disable-multilib --enable-languages=c,c++ --disable-libgcj
Thread model: posix
gcc version 4.10.0 20140610 (experimental) (GCC)
Cheers,
Nicholas Ormrod
RE: [PATCH, cpp] Fix line directive bug
Hello all, (Re-adding gcc-patches, since it got dropped and missed six emails) === CPP FEATURE SUGGESTION === Adding line directives inside of a macro expansion to differentiate between system tokens and user tokens is a valid solution. As Manuel pointed out, there would need to be many line directives. So, if "#define FOO(x) (x) + 1/0" was in a system file, and was instantiated from a user file like "FOO(2/0)", the "2/0" are user tokens (which should raise a div-by-zero warning), but the "1/0" are system tokens (and so should not raise a warning). This would need to expand as follows: # 1 "user.cpp" 3 ( # 1 "user.cpp" 2/0 # 1 "user.cpp" 3 ) + 1/0 # 2 "user.cpp" The last line directive is crucial, to specify that the rest of the file is not comprised of user tokens. Currently, compiling a user program with "FOO(2/0)" yields a single div-by-zero error. Compiling that same program with -no-integrated-cpp yields two errors: the original and correct 2/0 error, but also the 1/0 system error. So adjusting the preprocessor to emit extra line directives would be consistent with the integrated-cpp mode. === NEXT STEPS === The bugzilla report now has two independent reports, so this is an issue which is currently affecting the community. Both reports, it should be noted, stem from the use of ccache, which caches preprocessed output (since this bug is not present with integrated preprocessing). The status quo is subtle and infrequent, but confounding. The extremely specific requirements to trigger the bug mean that it is effectively undiagnosable (my initial bug report was the result of a very chance set of circumstances and a lot of elbow grease, which ended up being the reason for a years-old abnormality in our error messages). Further, when the bug does happen, it is severely detrimental, since it disables warnings in the rest of the file. Given these two circumstances, undiagnosability and high-impact, I think that a fix should be applied immediately. We currently have two possible approaches. I see the trade-offs between these two solutions as follows: Adding full line directives is not implemented, and will be more complicated than my patch. It will require someone else to implement it, but should fix the problem in its entirety. My solution is simple, battle-tested (I've had it in our production gcc for a few months now), and ready to roll. The primary downside is that it will not supress system-token errors in macro expansions (though in my experience this is not a problem). Cheers, Nicholas
RE: [PATCH] PR preprocessor/60723 - missing system-ness marks for macro
Hello Dodji, I found time this morning to run your changes through our system. I patched our gcc-4.8.1 with your latest change, and ran it through our folly testsuite. One thing that I immediately noticed was that this increased the preprocessed size substantially. When preprocessing my favorite .cpp file, its .ii grew from 137k lines to 145k, a 5% increase. All the folly code compiled and ran successfully under the changes. I looked at some of the preprocessed output. I was pleased to see that consecutive macros that expanded entirely to system tokens did not insert unnecessary line directives between them. I did, however, notice that __LINE__ was treated as belonging to the calling file, even when its token appears in the system file. That is to say: CODE: // system macro #define FOO() sys_token __LINE__ sys_token // non-system callsite FOO() // preprocessed output # 3 "test.cpp" 3 4 sys_token # 3 "test.cpp" 3 # 3 "test.cpp" 3 4 sys_token :CODE This seems to generalize to other builtin macros, like __FILE__. Otherwise, the code looks fine. There is only one thing I noticed: > + if (do_line_adjustments > + && !in_pragma > + && !line_marker_emitted > + && print.prev_was_system_token != !!in_system_header_at(loc)) > + /* The system-ness of this token is different from the one > + of the previous token. Let's emit a line change to > + mark the new system-ness before we emit the token. */ > + line_marker_emitted = do_line_change (pfile, token, loc, false); This line_marker_emitted assignment is immediately overwritten, two lines below. However, from a maintainability perspective, this is probably a good assignment to keep. > cpp_output_token (token, print.outf); > + line_marker_emitted = false; > } Thanks for this diff! Cheers, Nicholas
RE: [PATCH] PR preprocessor/60723 - missing system-ness marks for macro
The fact that extra line directives are inserted around built-in tokens isn't ideal, but I must concur with Dodji's assessment that such a fix belongs in a separate patch. The purpose of this patch is to resolve a discrepancy between integrated-cpp and non-integrated-cpp. The locations of built-in tokens is odd, but consistent. It is an orthogonal issue, and therefore should not be fixed as part of this diff. In terms of practical considerations, the impact of the extra line directives is minimal. The main concern of the two reporters (see gcc.gnu.org/bugzilla/show_bug.cgi?id=60723) is resolved by this patch, and is completely unaffected by the extra directives. I think that this patch is good to go. Regards, Nicholas P.S. Dodji, if you are also going to fix the locations of built-in tokens, it would also be worth adjusting their expansion locations. As mentioned in the bug report, built-in tokens are expanded at the closing paren of a macro call, whereas non-built-in tokens are expanded at the opening paren. This is weird.
