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

Add assignment and floating-point constant attributes #171

Merged
merged 11 commits into from
Jul 30, 2024
Merged

Conversation

sim642
Copy link
Member

@sim642 sim642 commented Jul 25, 2024

Closes #168. This requires parsing both assignment expressions and floating-point constant expressions in attribute parameters.

These attributes also seem to be used in newer headers in older MacOS now: #154 (comment).

@sim642 sim642 added the bug label Jul 25, 2024
@sim642 sim642 requested a review from vesalvojdani July 25, 2024 11:26
@sim642 sim642 added this to the 2.0.4 milestone Jul 25, 2024
@sim642
Copy link
Member Author

sim642 commented Jul 25, 2024

This doesn't seem to be enough to pass MacOS tests but from the garbage output I cannot tell which test is failing even. And yet it's completely uninformative: to debug any issues one must download the log (which is different from the printed output!) from GitHub Actions and still guess what's wrong (and make GitHub Actions upload more files like stdio.h to see what cannot be parsed then).

@sim642
Copy link
Member Author

sim642 commented Jul 25, 2024

In some cases this seems to be due to MacOS using GCC 14, which turned some warnings into errors: https://gcc.gnu.org/gcc-14/porting_to.html#warnings-as-errors.

@sim642 sim642 self-assigned this Jul 25, 2024
@sim642
Copy link
Member Author

sim642 commented Jul 25, 2024

Ok, float constants are not the right thing because there's also this

int ptsname_r(int fildes, char *buffer, size_t buflen) __API_AVAILABLE(macos(10.13.4), ios(11.3), tvos(11.3), watchos(4.3));

I'm not sure what 10.13.4 is supposed to be in the lexer...
Clang itself has a massive hack for this part:
https://github.com/llvm/llvm-project/blob/abc2eae68290c453e1899a94eccc4ed5ea3b69c1/clang/lib/Parse/ParseDecl.cpp#L1174-L1177

  // Parse the major (and possibly minor and subminor) versions, which
  // are stored in the numeric constant. We utilize a quirk of the
  // lexer, which is that it handles something like 1.2.3 as a single
  // numeric constant, rather than two separate tokens.

@sim642
Copy link
Member Author

sim642 commented Jul 26, 2024

I now implemented an equally bad hack: 10.13.4 is lexed as 10.13 and .4 which are both floating point literal tokens. Then the parser accepts such sequence of two floats and combines it.

Comment on lines +130 to +132
# Disable GCC 14 new warnings as errors because some tests contain them
CFLAGS += -Wno-error=implicit-int -Wno-error=implicit-function-declaration -Wno-error=int-conversion -Wno-error=incompatible-pointer-types

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens with old GCC versions? Do they already have these options?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I checked with GCC 7-13 (everything older we still look for in our RealGccConfigure) in Godbolt, that these already existed, but just weren't errors by default, so these shouldn't be a problem.
GCC 14 added a few new ones I had to exclude here (in order to avoid needing GCC-version specific configuration logic here).

@@ -657,7 +658,7 @@ sub addToGroup {
addBadComment("scott/globalprob", "Notbug. Not a bug if fails on a non-Linux machine ;-)");
addTest("scott/bisonerror $gcc");
addTest("scott/cmpzero");
addTest("scott/kernel1 $gcc");
# addTest("scott/kernel1 $gcc");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this test no longer of interest? I would guess McPeak had a reason for adding it? In what way does it fail if it is left there?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It fails on GCC 14 with some of the new error types that aren't so easy to suppress in backwards-compatible way.

The main line of the test is

DECLARE_WAIT_QUEUE_HEAD(log_wait);

which comes from the Linux kernel, where DECLARE_WAIT_QUEUE_HEAD is supposed to be a macro (from some unknown kernel version).
This test doesn't have the macro or any includes, so this is being parsed as some very old C function declaration that declares a function without specifying its return type and also declares a new type log_wait in the middle of the function declaration.

So this test has never been about parsing actual kernel sources (which have the macro that expands to something else entirely). In it, CIL is just successfully parsing some unpreprocessed code in a completely different way from its original meaning.
CIL will still support these old C function declarations, it's just that this test isn't enabled any more.

This could be made conditional to not run on GCC 14+ (or run with different CFLAGS), but I think detecting the GCC version from gcc --version in Makefile is awfully error-prone, so it's not worth the trouble for what is a misguided test to begin with.

Copy link
Member

@michael-schwarz michael-schwarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! The hack is a bit ugly but I also don't really see a way around. Luckily, it is constrained to the attribute language, so I don't think it's too bad.

@sim642 sim642 merged commit 30781b0 into develop Jul 30, 2024
54 of 56 checks passed
@sim642 sim642 deleted the attr-assign branch July 30, 2024 07:33
sim642 added a commit to sim642/opam-repository that referenced this pull request Jul 30, 2024
CHANGES:

* Add `Return` statement expression location (goblint/cil#167).
* Add `availability` attribute support (goblint/cil#168, goblint/cil#171).
* Remove unused `Libmaincil` module (goblint/cil#165).
* Fix some synthetic locations (goblint/cil#166, goblint/cil#167).
avsm pushed a commit to avsm/opam-repository that referenced this pull request Sep 5, 2024
CHANGES:

* Add `Return` statement expression location (goblint/cil#167).
* Add `availability` attribute support (goblint/cil#168, goblint/cil#171).
* Remove unused `Libmaincil` module (goblint/cil#165).
* Fix some synthetic locations (goblint/cil#166, goblint/cil#167).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Everything fails on MacOS 14 because of attributes with assignment in time.h
2 participants