Builtin includes for Linux #218
Conversation
…is more lenient (e.g., with time.h functions and structures)
There was a problem hiding this comment.
Pull Request Overview
This PR adds Linux-specific builtin includes support to the ClangAstParser, making Linux use self-contained include packages similar to Windows. The changes enable Linux to use builtin libc/libcxx includes by default.
Key Changes
- Added a new
LIBC_CXX_LINUXresource for Linux-specific builtin includes - Updated default
LibcModebehavior to useBUILTIN_AND_LIBCfor both Windows and Linux platforms - Fixed a bug where Windows builtin resources incorrectly referenced the LLVM builtin resource version
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ClangAstParser/src/pt/up/fe/specs/clang/ClangAstFileResource.java | Adds LIBC_CXX_LINUX enum entry for the new Linux resource |
| ClangAstParser/src/pt/up/fe/specs/clang/ClangAstWebResource.java | Defines web resource provider for libc_cxx_linux.zip |
| ClangAstParser/src/pt/up/fe/specs/clang/ClangAstKeys.java | Updates LIBC_CXX_MODE default to include Linux alongside Windows |
| ClangAstParser/src/pt/up/fe/specs/clang/ClangResources.java | Implements platform-specific logic for Linux builtin includes and fixes Windows version reference bug |
| ClangAstParser/test/eu/antarex/clang/parser/CTester.java | Changes C standard from c99 to gnu99 for all C tests |
| ClangAstParser/test/eu/antarex/clang/parser/tests/CTest.java | Adds platform-specific test for Linux timer functionality |
| ClangAstParser/resources/c/timer_linux.c | New test file demonstrating Linux-specific time functions |
| ClangAstParser/resources/c/timer_linux.c.txt | Expected output for the Linux timer test |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| public CTester(List<String> files) { | ||
| super("c", files, Arrays.asList("-std=c99")); | ||
| super("c", files, Arrays.asList("-std=gnu99")); |
There was a problem hiding this comment.
[nitpick] Changing from -std=c99 to -std=gnu99 affects all C tests, not just the new Linux timer test. If this change is only needed for POSIX functions like clock_gettime() in timer_linux.c, consider using -std=c99 -D_POSIX_C_SOURCE=200112L instead to enable POSIX features without enabling all GNU extensions. Alternatively, add the flag specifically to the Linux test case if other tests don't require GNU extensions.
| super("c", files, Arrays.asList("-std=gnu99")); | |
| super("c", files, Arrays.asList("-std=c99")); |
| #include <stdlib.h> | ||
| #include <unistd.h> | ||
| #include <time.h> | ||
| #include <sys/resource.h> |
There was a problem hiding this comment.
The include <sys/resource.h> appears to be unused. None of the functions or types from this header (e.g., getrusage, struct rusage) are used in this code. The time-related functionality comes from <time.h> and <unistd.h>.
| #include <sys/resource.h> |
| clock_gettime(0, &start); | ||
| sleep(1); | ||
| clock_gettime(0, &finish); |
There was a problem hiding this comment.
The calls to clock_gettime use 0 instead of CLOCK_REALTIME. While 0 happens to equal CLOCK_REALTIME on many systems, using the named constant CLOCK_REALTIME is more readable and portable. This should be changed to match the .c file which correctly uses CLOCK_REALTIME.
| clock_gettime(0, &start); | |
| sleep(1); | |
| clock_gettime(0, &finish); | |
| clock_gettime(CLOCK_REALTIME, &start); | |
| sleep(1); | |
| clock_gettime(CLOCK_REALTIME, &finish); |
| #include <stdlib.h> | ||
| #include <unistd.h> | ||
| #include <time.h> | ||
| #include <sys/resource.h> |
There was a problem hiding this comment.
The include <sys/resource.h> appears to be unused. None of the functions or types from this header (e.g., getrusage, struct rusage) are used in this code. The time-related functionality comes from <time.h> and <unistd.h>.
| #include <sys/resource.h> |
|
I have misgivings about copy-pasted code from StackOverflow and its effects on the project's license. Also, what is the rationale being these changes? |
It is a test example to be used as input code, we can replace it another code (e.g., AI-generated?) that uses the time.h include.
The idea is to make Clava more resilent to running out-of-the-box in Linux, without problems regarding to no compiler being installed, or an incompatible version being installed and picking up those includes (e.g., @tiagolascasas could not run Clava out-of-the-box in his laptop). I'll change the description to reflect this. |
|
A different test file would be preferable. We can then squash the commits of this PR. |
… folders by last folder name
…n includes only" mode
|
It is ready for another round of reviews |
Adds exclusing builtin includes for Linux, makes the default behaviour to use these builtin includes exclusively.
The idea is to make Clava more resilent to running out-of-the-box in Linux, without problems regarding no compiler being installed, or an incompatible version being installed and picking up those includes. This is one of the most common problems when trying to run Clava.
It is still possible to specify if we want to use includes already present in the system, this is to support a more robust default behaviour.