Adaptation of `pallene-tracer` directly in Pallene #614

singul4ri7y · 2024-07-09T12:09:33Z

Resovles #602

We do not need -Wpedantic as we are specifying C99 standard. According to GCC, -Wpedantic will generate warnings based on strict ISO C/ANSI C, which is basically C89. If we really need to use some features from C89 (ISO C), we may use flags specific to the feature we want to use e.g. -Wdeclaratoin-after-statement to lift all the variable declarations at the top of the function definition etc.

hugomg

We also need to edit the README to tell our users how to install the ptracer dependency.

hugomg · 2024-07-09T12:38:36Z

run-tests

@@ -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'


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.

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.

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.

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.

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=.

hugomg · 2024-07-09T12:51:29Z

src/pallene/c_compiler.lua

@@ -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"


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.

srijan-paul

LGTM!
I just want to install it locally once.
Can the pallene tracer repo be updated before we merge this?

srijan-paul · 2024-07-10T19:32:19Z

src/pallene/pallenelib.lua

@@ -46,6 +46,10 @@ return [==[
 #include <stdbool.h>
 #include <stdlib.h>

+/* Pallene Tracer for tracebacks (dynamically linked). */
+/* Look at https://github.com/pallene-lang/pallene-tracer for more info. */


This repo is still empty, by the way.
Is that intentional?

Switch to the dev branch. And also please review the PR which intends to merge the dev branch to main in pallene-tracer.

hugomg · 2024-08-03T22:03:40Z

.github/workflows/ci.yml

@@ -77,6 +77,13 @@ jobs:
                  make
                  sudo make install

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


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

Let's do that when Pallene Tracer is stabilized.

hugomg · 2024-08-03T22:04:27Z

src/pallene/c_compiler.lua

@@ -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",


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

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.

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.

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

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

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

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.

srijan-paul

looking good.
Just a few minor changes.

src/pallene/pallenec.lua

hugomg

I still have reservations about the rpath bit. But we can address that in a future PR.

singul4ri7y · 2024-08-07T13:10:09Z

@hugomg Agree on the rpath part. But if distros like Arch Linux don't resolve the /usr/local/lib directory by default, it's hard to say most distros will, especially distros like Manjaro, EndevourOS or SteamOS for that matter (Linux gaming is a thing btw), derived directly from ArchLinux. I would like to keep it for now, until we find a better alternative, which we should.

Suggested changes has been addressed.

singul4ri7y added 2 commits July 9, 2024 17:57

Adaption of pallene-tracer into Pallene

7bc5fe2

singul4ri7y marked this pull request as draft July 9, 2024 12:47

hugomg requested changes Jul 9, 2024

View reviewed changes

srijan-paul requested changes Jul 10, 2024

View reviewed changes

Pallene Tracer adoption and CI script update

bdfd1eb

singul4ri7y force-pushed the stack-trace-adaption branch from 2945afa to bdfd1eb Compare August 3, 2024 15:03

singul4ri7y added 3 commits August 3, 2024 21:04

CI script typo fix

890999b

CI script fix, fingers crossed

a1b85e1

Remove Pallene Tracer installation for linting check in CI

906f8b3

hugomg requested changes Aug 3, 2024

View reviewed changes

Pallene Tracer API usage improvements

ec66bf2

srijan-paul previously requested changes Aug 5, 2024

View reviewed changes

src/pallene/pallenec.lua Outdated Show resolved Hide resolved

src/pallene/pallenec.lua Outdated Show resolved Hide resolved

singul4ri7y added 2 commits August 5, 2024 14:53

Adaptation of inline function support of Pallene Tracer

ff1cdb0

Traceback API fix

a329f07

hugomg approved these changes Aug 7, 2024

View reviewed changes

hugomg marked this pull request as ready for review August 7, 2024 12:04

singul4ri7y force-pushed the stack-trace-adaption branch from 5083077 to a329f07 Compare August 7, 2024 13:45

singul4ri7y and others added 2 commits August 7, 2024 19:49

Merge branch 'master' into stack-trace-adaption

e524558

Address changes suggested by @srijan-paul

d1617da

singul4ri7y merged commit 516b397 into master Aug 8, 2024
2 checks passed

singul4ri7y deleted the stack-trace-adaption branch August 8, 2024 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adaptation of `pallene-tracer` directly in Pallene #614

Adaptation of `pallene-tracer` directly in Pallene #614

singul4ri7y commented Jul 9, 2024 •

edited

Loading

hugomg left a comment

hugomg Jul 9, 2024

singul4ri7y Jul 9, 2024

hugomg Jul 9, 2024

singul4ri7y Jul 9, 2024

singul4ri7y Jul 9, 2024

hugomg Jul 9, 2024

srijan-paul left a comment

srijan-paul Jul 10, 2024

singul4ri7y Jul 10, 2024 •

edited

Loading

hugomg Aug 3, 2024

singul4ri7y Aug 3, 2024

hugomg Aug 3, 2024

singul4ri7y Aug 3, 2024

hugomg Aug 4, 2024

singul4ri7y Aug 4, 2024 •

edited

Loading

hugomg Aug 4, 2024

singul4ri7y Aug 4, 2024

hugomg Aug 4, 2024

singul4ri7y Aug 4, 2024

singul4ri7y Aug 4, 2024 •

edited

Loading

srijan-paul left a comment

hugomg left a comment

singul4ri7y commented Aug 7, 2024

Adaptation of pallene-tracer directly in Pallene #614

Adaptation of pallene-tracer directly in Pallene #614

Conversation

singul4ri7y commented Jul 9, 2024 • edited Loading

hugomg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srijan-paul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singul4ri7y Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singul4ri7y Aug 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singul4ri7y Aug 4, 2024 • edited Loading

Choose a reason for hiding this comment

srijan-paul left a comment

Choose a reason for hiding this comment

hugomg left a comment

Choose a reason for hiding this comment

singul4ri7y commented Aug 7, 2024

Adaptation of `pallene-tracer` directly in Pallene #614

Adaptation of `pallene-tracer` directly in Pallene #614

singul4ri7y commented Jul 9, 2024 •

edited

Loading

singul4ri7y Jul 10, 2024 •

edited

Loading

singul4ri7y Aug 4, 2024 •

edited

Loading

singul4ri7y Aug 4, 2024 •

edited

Loading