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.

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

Incorrect line directives after #include line #604

Closed
squeek502 opened this issue Dec 17, 2023 · 4 comments · Fixed by #608
Closed

Incorrect line directives after #include line #604

squeek502 opened this issue Dec 17, 2023 · 4 comments · Fixed by #608
Labels
bug Something isn't working

Comments

@squeek502
Copy link
Contributor

squeek502 commented Dec 17, 2023

Minimal test case, unsure exactly what else this affects (if anything):

test.rc:

#include "a.h"
foo

a.h is an empty file


arocc -E -fuse-line-directives --emulate=msvc test.rc

produces

#line 1 "test.rc"
#line 1 "<builtin>"
#line 294 "<builtin>"
#line 1 "<command line>"
#line 1 "test.rc"
#line 1 "./a.h"

#line 3 "test.rc"
foo

There is no line 3 of test.rc, so that last line directive should be for line 2. Note that adding more #include lines does not seem to compound the effect (it's still off-by-one).


Clang output for comparison (clang -E -fuse-line-directives -xc test.rc):

#line 1 "test.rc"
#line 1 "<built-in>"
#line 1 "<built-in>"
#line 398 "<built-in>"
#line 1 "<command line>"
#line 1 "<built-in>"
#line 1 "test.rc"
#line 1 "./a.h"
#line 2 "test.rc"
foo
@Vexu Vexu added the bug Something isn't working label Dec 17, 2023
@squeek502
Copy link
Contributor Author

squeek502 commented Dec 28, 2023

This patch fixes this particular case:

diff --git a/src/aro/Preprocessor.zig b/src/aro/Preprocessor.zig
index a70c739..e850c62 100644
--- a/src/aro/Preprocessor.zig
+++ b/src/aro/Preprocessor.zig
@@ -2966,7 +2966,7 @@ fn include(pp: *Preprocessor, tokenizer: *Tokenizer, which: Compilation.WhichInc
         if (next.id != .nl) break;
         tokenizer.* = tmp;
     }
-    try pp.addIncludeResume(next.source, next.end, next.line);
+    try pp.addIncludeResume(next.source, next.end, first.line);
 }
 
 /// tokens that are part of a pragma directive can happen in 3 ways:

but it's pretty clearly a hack. @ehaas could you provide some insight on what the intention of next in this code is?

arocc/src/aro/Preprocessor.zig

Lines 2962 to 2969 in e382265

var next = first;
while (true) {
var tmp = tokenizer.*;
next = tmp.nextNoWS();
if (next.id != .nl) break;
tokenizer.* = tmp;
}
try pp.addIncludeResume(next.source, next.end, next.line);

@squeek502
Copy link
Contributor Author

squeek502 commented Dec 28, 2023

Another manifestation of this problem without #include:

test.c:

this is line 1








this is line 10
$ arocc -E -fuse-line-directives test.c
#line 1 "test.c"
#line 1 "<builtin>"
#line 294 "<builtin>"
#line 1 "<command line>"
#line 1 "test.c"
this is line 1
#line 11 "test.c"
this is line 10

(the #line 11 "test.c" should be #line 10 "test.c")

@ehaas
Copy link
Collaborator

ehaas commented Dec 29, 2023

I'm not totally sure what next is doing there - seems like all the tests still pass if that is removed (and just use first for everything in the addIncludeResume call), and it fixes the .rc test you provided.

@squeek502
Copy link
Contributor Author

squeek502 commented Dec 29, 2023

I believe it's skipping past newlines after an include, so e.g.

#include "foo.h"


foo

would end up as

#line 1 "foo.h"

#line 4 "root.c"
foo

instead of

#line 1 "foo.h"

#line 2 "root.c"


foo

In looking into this more, though, I'm confused about whether or not this is actually true:

arocc/src/aro/Preprocessor.zig

Lines 3133 to 3134 in e382265

// line_no is 0 indexed
try w.print(" {d} \"", .{line_no + 1});

In printing out the tokens, the line numbers all seem to actually be 1-indexed. The only 0-based tokens I see are the inserted include_start tokens. So, I think that's the bug here--the line number is not actually 0-indexed, and the start tokens are incorrectly being added with 0-indexed line numbers instead of 1-indexed like all the rest of the tokens.

squeek502 added a commit to squeek502/preresinator that referenced this issue Dec 29, 2023
Line numbers are 1-indexed, not 0-indexed. Before this commit, include_start/include_resume tokens were being added with a mix of 1-indexed and 0-indexed line numbers. After this commit, all line numbers are 1-indexed.

Closes Vexu#604
@Vexu Vexu closed this as completed in #608 Dec 30, 2023
Vexu pushed a commit that referenced this issue Dec 30, 2023
Line numbers are 1-indexed, not 0-indexed. Before this commit, include_start/include_resume tokens were being added with a mix of 1-indexed and 0-indexed line numbers. After this commit, all line numbers are 1-indexed.

Closes #604
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants