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.

Sign up for GitHub

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

Adaptation of pallene-tracer directly in Pallene #614

Merged
merged 11 commits into from
Aug 8, 2024
Merged

Adaptation of pallene-tracer directly in Pallene #614

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7bc5fe2
Adaption of pallene-tracer into Pallene
singul4ri7y Jul 9, 2024
8d99ce3
Remove -Wpedantic flag from run-tests script
singul4ri7y Jul 9, 2024
bdfd1eb
Pallene Tracer adoption and CI script update
singul4ri7y Aug 3, 2024
890999b
CI script typo fix
singul4ri7y Aug 3, 2024
a1b85e1
CI script fix, fingers crossed
singul4ri7y Aug 3, 2024
906f8b3
Remove Pallene Tracer installation for linting check in CI
singul4ri7y Aug 3, 2024
ec66bf2
Pallene Tracer API usage improvements
singul4ri7y Aug 4, 2024
ff1cdb0
Adaptation of inline function support of Pallene Tracer
singul4ri7y Aug 5, 2024
a329f07
Traceback API fix
singul4ri7y Aug 6, 2024
e524558
Merge branch 'master' into stack-trace-adaption
singul4ri7y Aug 7, 2024
d1617da
Address changes suggested by @srijan-paul
singul4ri7y Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pallene-dev-1.rockspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ dependencies = {
"lua == 5.4",
"lpeg >= 1.0",
"argparse >= 0.7.0",
"pallene-tracer >= 0.5.0a"
}
external_dependencies = {
PTRACER = {
header = "ptracer.h",
}
}
build = {
type = "builtin",
Expand Down
2 changes: 1 addition & 1 deletion run-tests
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ FLAGS=(--verbose --no-keep-going)

# To speed things up, we tell the C compiler to skip optimizations. (It's OK, the CI still uses -O2)
# Also, add some compiler flags to verify standard compliance.
export CFLAGS='-O0 -std=c99 -Wall -Werror -Wundef -Wpedantic -Wno-unused'
export CFLAGS='-O0 -std=c99 -Wall -Werror -Wundef -Wno-unused'
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure we want to get rid of -Wpedantic? If I remember correctly, it's not just for C89. In this case it would know about c99 and warn about things that are not c99.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a long and complicated history behind it that I have been researching thyself for a while.

Generally, for some time being ISO was in charge of publishing C standards (Like C89, C90, C95 and C99). People easily mistakes ISO standards as pure C standards (they are not wrong tho). But there is a catch. When it is said ISO C, it literally means C89 standard initially developed by ISO. It's because C89 is said to be the most complete C standard ever developed and other C standards are just inherited C89 with some extra features minus some strictness. The C89 standard is also said to be ANSI C by historical mistake (don't quote me on that).

According to GCC manual, -Wpedantic will generate warnings as if you are using C89. So, using it beside C99 actually doesn't make all that sense instead of using -std=c89 or -pedantic or -ansi flags to fully adopt C89. But with that being said as all the standards are sorta inheritance of C89, there are some flags we can use to take advantage of some features of C89 aka ISO C. One of the flags that I came across recently while patching DWL (DWM for Wayland) is -Wdeclaration-after-statement which uses the ISO C suggestion for keeping all the local variable declaration at the top.

For the record, using gcc:

  • Without any -std defaults to latest GNU C standard equivalent of -std=gnu**.
  • With -std=c** to use any pure C standard.
  • With -pedantic or -ansi to use ISO C89, which is less robust way to define -std=c89.
  • With -Wpedantic to generate warnings as if we are using ISO C89 even tho we may have some standard defined. Using -Wpedantic with -Werror is lesser of a robust way of using -std=c89.

Copy link
Member

Choose a reason for hiding this comment

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

From the GCC manual:

-std=

Determine the language standard. This option is currently only supported when compiling C or C++.
The compiler can accept several base standards, such as c90 or c++98, and GNU dialects of those
standards, such as gnu90 or gnu++98. When a base standard is specified, the compiler accepts all
programs following that standard plus those using GNU extensions that do not contradict it. For
example, -std=c90 turns off certain features of GCC that are incompatible with ISO C90, such as the
"asm" and "typeof" keywords, but not other GNU extensions that do not have a meaning in ISO C90, such
as omitting the middle term of a "?:" expression. On the other hand, when a GNU dialect of a standard
is specified, all features supported by the compiler are enabled, even when those features change the
meaning of the base standard. As a result, some strict-conforming programs may be rejected. The
particular standard is used by -Wpedantic to identify which features are GNU extensions given that
version of the standard. For example -std=gnu90 -Wpedantic warns about C++ style // comments, while
-std=gnu99 -Wpedantic does not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From Wikipedia:

ANSI C, ISO C, and Standard C are successive standards for the C programming language published by the American National Standards Institute (ANSI) and ISO/IEC JTC 1/SC 22/WG 14 of the International Organization for Standardization (ISO) and the International Electrotechnical Commission (IEC). Historically, the names referred specifically to the original and best-supported version of the standard (known as C89 or C90). Software developers writing in C are encouraged to conform to the standards, as doing so helps portability between compilers.

This is from GCC flags manual:

-Wpedantic
-pedantic
Issue all the warnings demanded by strict ISO C and ISO C++; diagnose all programs that use forbidden extensions, and some other programs that do not follow ISO C and ISO C++. This follows the version of the ISO C or C++ standard specified by any -std option used.

Actually to be honest I am confused right now. From my experience, while using -Wpedantic with -std=c11 I was getting warnings which was of C89 standard as far as I can remember.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am bringing back -Wpedantic flag.

Here is where I was wrong: ISO C used to be historically and strictly referred to C89. But now it is not. ISO C standard in this case is the standard you specify using -std=.


if [ "$#" -eq 0 ]; then
if command -v parallel >/dev/null; then
Expand Down
3 changes: 3 additions & 0 deletions src/pallene/c_compiler.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ local c_compiler = {}

local CC = os.getenv("CC") or "cc"
local CFLAGS = os.getenv("CFLAGS") or "-O2"
local SHLIBLOC = os.getenv("SHLIBLOC") or "/usr/local/lib"
Copy link
Member

Choose a reason for hiding this comment

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

Pallene is installed via Luarocks, which checks that ptracer is installed. If Luarocks only checks default installation locations, then there would be nothing to gain by adding this configuration flag. And if we do need a configure flag, we should follow whatever scheme Luarocks uses (presumably it would use a ptracer-specific variables instead of a global sharedlib variable).

I think that for now we can just leave this out and assume people will install to default locations. We can add configuration later if it comes up.


local function get_uname()
local ok, err, uname = util.outputs_of_execute("uname -s")
Expand Down Expand Up @@ -61,6 +62,8 @@ function c_compiler.compile_o_to_so(in_filename, out_filename)
CFLAGS_SHARED,
"-o", util.shell_quote(out_filename),
util.shell_quote(in_filename),
"-lptracer",
"-Wl,-rpath="..SHLIBLOC,
})
end

Expand Down
Loading
Loading