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

Linemarkers proof of concept #517

Merged
merged 12 commits into from
Oct 9, 2023
Merged

Linemarkers proof of concept #517

merged 12 commits into from
Oct 9, 2023

Conversation

ehaas
Copy link
Collaborator

@ehaas ehaas commented Oct 2, 2023

Tests need to be fixed and #line output support needs to be added. Also need to figure out how we'll handle the 3 and 4 flags - should those be fields in Source that track what kind of headers they are?

`3'
This indicates that the following text comes from a system header file, so certain warnings should be suppressed.
`4'
This indicates that the following text should be treated as being wrapped in an implicit extern "C" block.

@Vexu
Copy link
Owner

Vexu commented Oct 2, 2023

It can be an enum since 4 implies 3: (from clang)

/// HandleDigitDirective - Handle a GNU line marker directive, whose syntax is
/// one of the following forms:
///
///     # 42
///     # 42 "file" ('1' | '2')?
///     # 42 "file" ('1' | '2')? '3' '4'?
///

@ehaas
Copy link
Collaborator Author

ehaas commented Oct 3, 2023

Should be some improved line numbers now. I still need to figure out what causes clang/gcc to output line number directives within a file

@Vexu
Copy link
Owner

Vexu commented Oct 4, 2023

I was planning on doing that by replacing whitespace tokens with more than three newlines with a line marker pointing to the previous line of the next token.

@ehaas
Copy link
Collaborator Author

ehaas commented Oct 4, 2023

I'm going to be mostly offline for the next 5 days and might not have time to finish this up, so if you want to take what I have or rework it or take a different approach, please feel free.

@Vexu Vexu marked this pull request as ready for review October 4, 2023 20:16
@Vexu
Copy link
Owner

Vexu commented Oct 4, 2023

I've wanted this feature for a long time but haven't gotten around to implementing it so it's great to finally have it.

@ehaas
Copy link
Collaborator Author

ehaas commented Oct 4, 2023

I think the newline tracking doesn't work if a single line has multiple sources with different lines - e.g. if I have a source file with just #include <stddef.h> my -E output looks like:

# 7 "/Users/ehaas/source/arocc/include/stddef.h" 3
typedef # 70 "<builtin>"
long # 7 "/Users/ehaas/source/arocc/include/stddef.h" 3
ptrdiff_t;
typedef # 71 "<builtin>"
unsigned long # 8 "/Users/ehaas/source/arocc/include/stddef.h" 3
size_t;
typedef # 72 "<builtin>"
int # 9 "/Users/ehaas/source/arocc/include/stddef.h" 3
wchar_t;

(because stddef.h has typedef __PTRDIFF_TYPE__ ptrdiff_t; on line 7 but __PTRDIFF_TYPE__ is defined as long in <builtin> on line 70.

@squeek502
Copy link
Contributor

Nearly perfect on the test set I'm using now:

1 .rc files processed with discrepancies
different .res outputs:     1
unexpected compile errors:  0
missing compile errors:     0

484 .rc files processed without discrepancies
identical .res outputs:     459
expected compile errors:    25

Note that I also use line directives for error message reporting, so I'm sure I'll have follow-ups regarding the correctness of the line numbers given, but that doesn't affect correctness in terms of the compiled .res output of resinator.


Minimal reproduction of the problem in the one failing test case:

a.c:

#include "c.h"
THIS_IS_IN_A_DOT_C
#include "b.h"

b.h:

#pragma once
#include "c.h"
THIS_IS_IN_B_DOT_H

c.h:

#pragma once
THIS_IS_IN_C_DOT_H

Results in (via arocc -E -fuse-line-directives a.c):

#line 1 "a.c"
#line 1 "<builtin>"
#line 1 "<command line>"
#line 1 "a.c"
#line 1 ".\c.h"
THIS_IS_IN_C_DOT_H
#line 2 "a.c"
THIS_IS_IN_A_DOT_C
#line 1 ".\b.h"
#line 1 ".\c.h"
THIS_IS_IN_B_DOT_H
#line 4 "a.c"

(THIS_IS_IN_B_DOT_H is misreported as being in c.h)

Here's what clang (-E -xc -fuse-line-directives) gives:

#line 1 "a.c"
#line 1 "<built-in>"
#line 1 "<built-in>"
#line 368 "<built-in>"
#line 1 "<command line>"
#line 1 "<built-in>"
#line 1 "a.c"
#line 1 "./c.h"

THIS_IS_IN_C_DOT_H
#line 2 "a.c"
THIS_IS_IN_A_DOT_C
#line 1 "./b.h"


THIS_IS_IN_B_DOT_H
#line 3 "a.c"

@squeek502
Copy link
Contributor

Are #line 0 "<scratch space>" directives being outputted intentional? Here's one way to get them to be outputted:

#define blank1
#define blank2
#define blank3
#define blank4
#define blank5
#define blank6
#define blank7
#if !defined(FOO)
BAR
#endif

(at least 7 blank (or otherwise removed during preprocessing, like #define) lines before the #if !defined(FOO) are necessary for this particular reproduction)

With arocc this results in:

#line 1 "scratch.rc"
#line 1 "<builtin>"
#line 0 "<scratch space>"
#line 1 "<command line>"
#line 1 "scratch.rc"
#line 0 "<scratch space>"
BAR

Expected (clang) output is:

#line 1 "scratch.rc"
#line 1 "<built-in>"
#line 1 "<built-in>"
#line 368 "<built-in>"
#line 1 "<command line>"
#line 1 "<built-in>"
#line 1 "scratch.rc"








BAR

@Vexu
Copy link
Owner

Vexu commented Oct 9, 2023

Works now:

#line 1 "b.c"
#line 1 "<builtin>"
#line 184 "<builtin>"
#line 1 "<command line>"
#line 1 "b.c"
#line 9 "b.c"
BAR

@squeek502
Copy link
Contributor

squeek502 commented Oct 9, 2023

Fantastic, resinator now has parity in the win32-samples-rc-tests when using arocc as the preprocessor:

Processed 485 .rc files

---------------------------
  resinator
---------------------------

485 .rc files processed without discrepancies
identical .res outputs:     460
expected compile errors:    25

---------------------------

@andrewrk
Copy link
Contributor

andrewrk commented Oct 9, 2023

Amazing. Does that mean everyone is on board with removing the clang dependency of resinator unconditionally?

@squeek502
Copy link
Contributor

squeek502 commented Oct 9, 2023

Amazing. Does that mean everyone is on board with removing the clang dependency of resinator unconditionally?

Yep, I'm good with that. I'm also now planning on embedding aro as the preprocessor in standalone resinator to drop the external preprocessor dependency there, too.

@Vexu Vexu merged commit 93fd3c9 into Vexu:master Oct 9, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants