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 6 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
8 changes: 8 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,13 @@ jobs:
make
sudo make install

- name: Install Pallene Tracer
run: |
git clone https://github.com/pallene-lang/pallene-tracer
Copy link
Member

Choose a reason for hiding this comment

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

We should create version tags in that repo, and import a particular version. Otherwise, Pallene builds might break if there new commits over there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's do that when Pallene Tracer is stabilized.

cd pallene-tracer
sudo make install
luarocks --local make

- name: Build
run: luarocks --local make

Expand All @@ -87,3 +94,4 @@ jobs:
run: |
eval "$(luarocks path)"
busted -o gtest -v ./spec

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
2 changes: 2 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 @@ -61,6 +61,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=/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.

Do we really need rpath? I'm worried about hardcoding user/local/lib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Member

Choose a reason for hiding this comment

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

Why yes? In my system, "/usr/local/lib" is part of the default search path. And presumably the user should have installed ptracer somewhere in their default search path. Also, that directory is configurable in the ptracer makefile, so we should not be hardcoding it here.

Copy link
Contributor Author

@singul4ri7y singul4ri7y Aug 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Removed the hardcoded part. I don't know whats up with the distro you are using, from my experience, you need some -rpath to be defined. And here, my experience includes, Manjaro, Debian 10 and later and ArchLinux (which I use BTW). It does not work for every distro, I can assure you that much.

Copy link
Member

Choose a reason for hiding this comment

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

Rpath should only be necessary if you're installing the shared library in a non-standard location, and don't want to use LD_LIBRARY_PATH.

You can check the search path in your distro by running ld --verbose | grep SEARCH_DIR | sed -e 's/; /\n/g'. On mine, it includes /usr/local/lib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It includes /usr/local/lib in my case as well, but still fails to link libptracer.so :)

Copy link
Member

Choose a reason for hiding this comment

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

Did you try removing the entire "-Wl,-rpath=/usr/local/lib" line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

@singul4ri7y singul4ri7y Aug 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For your convenience, I get this error while running the test suite after removing the line.

Failure → ./spec/execution_tests.lua @ 103
#c_backend / Exported functions / exports public functions
./spec/execution_tests.lua:89: command failed: lua '__test_c__script.lua' > '__test_c__output.txt'

stack traceback:
	./spec/execution_tests.lua:89: in upvalue 'run_test'
	./spec/execution_tests.lua:104: in function <./spec/execution_tests.lua:103>

lua: error loading module '__test_c__' from file './__test_c__.so':
	libptracer.so: cannot open shared object file: No such file or directory
stack traceback:
	[C]: in ?
	[C]: in function 'require'
	__test_c__script.lua:1: in main chunk
	[C]: in ?
●✱
1 success / 0 failures / 1 error / 0 pending : 0.211186 seconds

But it works if I explicitly mention /usr/local/lib with LD_LIBRARY_PATH, which is pain in the a*s.

})
end

Expand Down
4 changes: 4 additions & 0 deletions src/pallene/coder.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1802,6 +1802,10 @@ function Coder:generate_module_header()
table.insert(out, "/* This file was generated by the Pallene compiler. Do not edit by hand */")
table.insert(out, "")
table.insert(out, string.format("#define PALLENE_SOURCE_FILE %s", C.string(self.filename)))
if self.flags.use_traceback then
table.insert(out, "/* Enable Pallene Tracer debugging. */")
table.insert(out, "#define PT_DEBUG")
end
table.insert(out, "")

table.insert(out, "/* ------------------------ */")
Expand Down
Loading
Loading