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 another example #24

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 3 additions & 7 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,11 @@ PTLUA_LDLIBS = -llua -lm
# Compilation targets
# ===================

.PHONY: library examples tests all install uninstall clean
.PHONY: library tests all install uninstall clean

library: \
pt-lua

examples: library \
examples/fibonacci/fibonacci.so

tests: library \
spec/tracebacks/anon_lua/module.so \
spec/tracebacks/depth_recursion/module.so \
Expand All @@ -51,7 +48,7 @@ tests: library \
spec/tracebacks/multimod/module_b.so \
spec/tracebacks/singular/module.so

all: library examples tests
all: library tests

install: library
$(INSTALL_EXEC) pt-lua $(BINDIR)
Expand All @@ -62,15 +59,14 @@ uninstall:
rm -rf $(BINDIR)/pt-run

clean:
rm -rf pt-lua examples/*/*.so spec/tracebacks/*/*.so
rm -rf pt-run spec/tracebacks/*/*.so

%.so: %.c
$(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $(LIBFLAG) $< -o $@

pt-lua: pt-lua.c ptracer.h
$(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $(PTLUA_LDFLAGS) $< -o $@ $(PTLUA_LDLIBS)

examples/fibonacci/fibonacci.so: examples/fibonacci/fibonacci.c ptracer.h
spec/tracebacks/anon_lua/module.so: spec/tracebacks/anon_lua/module.c ptracer.h
spec/tracebacks/depth_recursion/module.so: spec/tracebacks/depth_recursion/module.c ptracer.h
spec/tracebacks/dispatch/module.so: spec/tracebacks/dispatch/module.c ptracer.h
Expand Down
27 changes: 27 additions & 0 deletions examples/connected-components/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Copyright (c) 2024, The Pallene Developers
# Pallene Tracer is licensed under the MIT license.
# Please refer to the LICENSE and AUTHORS files for details
# SPDX-License-Identifier: MIT

OUTPUT_FILE = component.so
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a single makefile for all the examples, instead of copy-pasted makefiles for everyone? These makefiles are too complicated to be copy-pasted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we did/could, what was the point of detaching them from the root Makefile? I don't find them complicated, atleast compared to the root Makefile that we have. A moderate Makefile is far better than having no Makefile at all.

Copy link
Member

Choose a reason for hiding this comment

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

The main point of detaching them from the root is because we want to compile the examples after we do make install for the library, because that way it tests that the compilation will also work for the users.

I'd be ok copy-pasting trivial makefiles but these makefiles are not trivial. For starters:

  1. Long list of cflags
  2. Non-portable ifneq, MAKECMDGOALS
  3. Multiple phony targets
  4. Pattern rules (despite building a single file)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was not the main point actually, you can compile examples after installation from root Makefile as well, it's not a big deal. The main point was, as Pallene Tracer have 2 modes (debug and release controlled by PT_DEBUG macro), we have to show users the techniques to use these modes in a Makefile.

I can remember you telling me Makefiles need some love? :3

Copy link
Member

Choose a reason for hiding this comment

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

  • Using makefile variables is more natural than if statements
  • I think we should keep the example makefiles as small as possible. For example, we can probably get rid of CFLAGS, the pattern rules, and much more.


CC = cc
Copy link
Member

Choose a reason for hiding this comment

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

You don't have to specify the C compiler variable

CFLAGS = -O2 -std=c99 -pedantic -Wall -Wextra -Wformat-security
LIBFLAGS = -fPIC -shared

ifneq ($(findstring debug, $(MAKECMDGOALS)), )
CFLAGS += -DPT_DEBUG
endif
Copy link
Member

Choose a reason for hiding this comment

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

if statements in makefiles are complicated and non-portable. Consider

  • Just turn on DPT_DEBUG all the time
  • Configure the DPT_DEBUG via variables

Another alternative is to recursivelly invoke make, just like Lua's makefile does for make linux. But IMO that might be too complicated for what we are doing here.

Copy link
Collaborator Author

@singul4ri7y singul4ri7y Aug 17, 2024

Choose a reason for hiding this comment

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

Just turn on DPT_DEBUG all the time

The main point of detaching the Makefiles was to let the users try the examples with and w/o debugging mode enabled. If we turn on PT_DEBUG all the time, it would be better to move the examples build to root Makefile, which just doesn't seem right.

Variables sound good though. Or maybe we can do some redundancy here (I just found out BSD and POSIX make do not support if statements).


.PHONY: all debug clean

all: $(OUTPUT_FILE)
debug: $(OUTPUT_FILE)

clean:
rm -rf $(OUTPUT_FILE)

%.so: %.c
$(CC) $(CFLAGS) $(LDFLAGS) $(LIBFLAGS) $< -o $@

%.c: ptracer.h
113 changes: 113 additions & 0 deletions examples/connected-components/component.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
/*
* Copyright (c) 2024, The Pallene Developers
* Pallene Tracer is licensed under the MIT license.
* Please refer to the LICENSE and AUTHORS files for details
* SPDX-License-Identifier: MIT
*/

#include <stdlib.h>

#define PT_IMPLEMENTATION
#include "ptracer.h"

/* ---------------- PALLENE TRACER LUA INERFACE ---------------- */
#define CON_LUA_FRAMEENTER(fnptr) \
PALLENE_TRACER_LUA_FRAMEENTER(L, fnstack, fnptr, \
lua_upvalueindex(1), _frame)
/* ---------------- PALLENE TRACER LUA INERFACE END ---------------- */

/* ---------------- PALLENE TRACER C INTERFACE ---------------- */

#define CON_C_FRAMEENTER() \
PALLENE_TRACER_GENERIC_C_FRAMEENTER(fnstack, _frame)

#define CON_C_SETLINE() \
PALLENE_TRACER_GENERIC_C_SETLINE(fnstack)

#define CON_C_FRAMEEXIT() \
PALLENE_TRACER_FRAMEEXIT(fnstack)
Copy link
Member

Choose a reason for hiding this comment

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

  • These would look less scary if the defines fit in the same line, without \
  • The PALLENE_TRACER_GENERIC names are way too long
  • Even nicer would be if the user didn't have to define macros and could use ptracer macros directly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • Sure.
  • LOUD names are less confusing in my opinion. @srijan-paul also feel the same way I presume.
  • Users can, but it would be sorta hassle. We have talked about this earlier, it's okay if users define their own macros, it's not huge boilerplate. Besides, the usual way to go would be put this separately in a header file in module code base, so most C translations don't even have to look at these mess (see the multimod test). Writing some simple boilerplate shouldn't hurt, as you are writing frameenter and frameexit repeatedly in every function.

Copy link
Member

Choose a reason for hiding this comment

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

My main issue with the names are

  1. The long names blend together. In PALLENE_TRACER_GENERIC_C_SETLINE, the common PALLENE_TRACER_GENERIC prefix takes almost all the space and the relevant SETLINE is a small part of the end.
  2. We expect to call SETLINE very often, and such a long name makes it inconvenient to type by hand (without defining a helper macro)
  3. One of the original ideas with the GENERIC macros was to have good defaults. We did that for LINE numbers, but not for the fnstack. If we enforced a default for how to use the fnstack, or if we allowed the user to specify a "FNSTACK" macro, then there would be no need for CON_C_SETLINE or CON_C_FRAMEEXIT.

Copy link
Collaborator Author

@singul4ri7y singul4ri7y Aug 18, 2024

Choose a reason for hiding this comment

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

They are not actually "issues" per-se, at-least to my eyes.

  1. Alternative way to do this would be to reduce PALLENE_TRACER_GENERIC to something shorter, inevitably changing the PALLENE_TRACER part, which is prefix for almost everything in Pallene Tracer. Doing something like PALLENE_TRACER_GCS would be silly.
  2. You are supposed to write some macros. Because no matter how hard we abstract, users have to define their own macros one way or the another for Pallene Tracer APIs because a lot of things are in users hands. Users hands won't fall off if they write some extra macros, not to mention they would be copy-pasting most of the part anyway.
  3. Way too much generalized. What if users want to name it "stack" instead of "fnstack"? What if they want to make fnstack global? Or want to pass it as an upvalue? If we do account for all the cases, it would make macro matters even complicated. We did the generic part for setlines because it is only and only __LINE__ + 1 for every users, which can be and should be generalized.

Copy link
Member

Choose a reason for hiding this comment

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

  1. We could use a shorter name like PTRACER_ or even just PT_
  2. It's unfortunate that even the simplest examples need boilerplate macros
  3. For fnstack, it might suffice to ask the user for a single macro that says the name of the fnstack variable. I'm also not against recommending a default name. The idea is to have good defaults for the common cases.


/* ---------------- PALLENE TRACER C INTERFACE END ---------------- */

/* This is one way to work with Pallene Tracer call-stack, but certainly not recommended because
this way you loose the flexibility of using multiple Lua states. */
pt_fnstack_t *fnstack;

static void internal_dfs(lua_State *L, int *visited, int node) {
CON_C_FRAMEENTER();

if(visited[node]) {
CON_C_FRAMEEXIT();
return;
}

visited[node] = 1;

lua_rawgeti(L, 1, node); /* get the connected adjacent nodes of current node */
CON_C_SETLINE();
int n = luaL_len(L, -1); /* this line may trigger error */
lua_pop(L, 1);

/* for all the adjacent nodes */
for(int i = 1; i <= n; i++) {
lua_rawgeti(L, 1, node);
lua_rawgeti(L, -1, i);
int adjacent_node = lua_tointeger(L, -1);
lua_pop(L, 2); /* avoid Lua value-stack overflow by not keeping values on the stack. */

CON_C_SETLINE();
internal_dfs(L, visited, adjacent_node);
}

CON_C_FRAMEEXIT();
}

static int find_connected_components(lua_State *L, int *visited, int n) {
CON_C_FRAMEENTER();

int result = 0;

for(int i = 1; i <= n; i++) {
if(!visited[i]) {
result++;
CON_C_SETLINE();
internal_dfs(L, visited, i);
}
}

CON_C_FRAMEEXIT();
return result;
}

static int find_connected_components_lua(lua_State *L) {
CON_LUA_FRAMEENTER(find_connected_components_lua);

if(!lua_istable(L, 1))
luaL_error(L, "expected the first argument to be table");
if(!lua_isinteger(L, 2))
luaL_error(L, "expected the second argument to be integer");

int nodes = lua_tointeger(L, 2); /* get total number of nodes */
int *visited = calloc(nodes + 1, sizeof(int)); /* Lua prefers 1 based indexing */

/* Dispatch and push the result. */
lua_pushinteger(L, find_connected_components(L, visited, nodes));

free(visited);
return 1;
}

int luaopen_component(lua_State *L) {
fnstack = pallene_tracer_init(L);

lua_newtable(L);
int table = lua_gettop(L);

/* ---- find_connected_components ---- */
/* `pallene_tracer_init` function pushes the frameexit finalizer to the stack. */
lua_pushvalue(L, -2); /* passing the finalizer object as upvalue is generally the way to go. */
lua_pushcclosure(L, find_connected_components_lua, 1);
lua_setfield(L, table, "find_connected_components");

return 1;
}
20 changes: 20 additions & 0 deletions examples/connected-components/main.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
local component = require "component"

local nodes = 12
local graph = {
{ 2 }, -- 1st node
Copy link
Member

Choose a reason for hiding this comment

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

Consider [1] = {2} to make the graph easier to read.

{ 1, 5 }, -- 2nd node
{ 5 }, -- 3rd node
{ 5 },
{ 2, 3, 4 },
{ 7, 8 },
{ 6 },
{ 6 },
{ 10, 12 },
{ 9, 11, 12 },
{ 10, 12 },
{ 9, 10, 11 } -- 12th node
};

local total_connected_components = component.find_connected_components(graph, nodes);
print(total_connected_components) -- expect: 3
27 changes: 27 additions & 0 deletions examples/fibonacci/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Copyright (c) 2024, The Pallene Developers
# Pallene Tracer is licensed under the MIT license.
# Please refer to the LICENSE and AUTHORS files for details
# SPDX-License-Identifier: MIT

OUTPUT_FILE = fibonacci.so

CC = cc
CFLAGS = -O2 -std=c99 -pedantic -Wall -Wextra -Wformat-security
LIBFLAGS = -fPIC -shared

ifneq ($(findstring debug, $(MAKECMDGOALS)), )
CFLAGS += -DPT_DEBUG
endif

.PHONY: all debug clean

all: $(OUTPUT_FILE)
debug: $(OUTPUT_FILE)

clean:
rm -rf $(OUTPUT_FILE)

%.so: %.c
$(CC) $(CFLAGS) $(LDFLAGS) $(LIBFLAGS) $< -o $@

%.c: ptracer.h
Copy link
Member

Choose a reason for hiding this comment

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

Where is this ptracer.h coming from? There should be no ptracer.h in the current directory.

6 changes: 3 additions & 3 deletions examples/fibonacci/main.lua
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
-- Please refer to the LICENSE and AUTHORS files for details
-- SPDX-License-Identifier: MIT

local N = tonumber(arg[1]) or 40

local fibonacci = require "fibonacci"
print(fibonacci.fib(40))
-- Uncomment this and trigger an error. You can debug using the 'pallene-debug' script.
-- print(fibonacci.fib(40.0))
print(fibonacci.fib(N))
32 changes: 32 additions & 0 deletions spec/examples_spec.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
-- Copyright (c) 2024, The Pallene Developers
-- Pallene Tracer is licensed under the MIT license.
-- Please refer to the LICENSE and AUTHORS files for details
-- SPDX-License-Identifier: MIT

local util = require "spec.util"

local function assert_example(example, expected_content)
local cdir = util.shell_quote("examples/"..example)
local ok, err, output_content, _ =
util.outputs_of_execute(string.format("cd %s && make --quiet && ../../pt-lua main.lua", cdir))
assert(ok, err)
assert.are.same(expected_content, output_content)

-- With Pallene Tracer tracebacks enabled
local ok, err, output_content, _ =
util.outputs_of_execute(string.format([[
cd %s
make clean --quiet
make debug --quiet
../../pt-lua main.lua ]], cdir))
assert(ok, err)
assert.are.same(expected_content, output_content)
Copy link
Member

Choose a reason for hiding this comment

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

Why are we compiling and asserting twice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The first one is w/o Tracebacks, the second one is with tracebacks. Testing twice to ensure we are not doing anything terribly wrong in Pallene Tracer. But there is a nit, we need to run the first assertion with normal system Lua.

Copy link
Member

Choose a reason for hiding this comment

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

Could those be two separate it blocks?

end

it("Fibonacci", function()
assert_example("fibonacci", "102334155\n")
end)

it("Find Connected Components", function()
assert_example("connected-components", "3\n")
end)
6 changes: 2 additions & 4 deletions spec/tracebacks_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,8 @@

local util = require "spec.util"

local function assert_test(example, expected_content)
assert(util.execute("make --quiet tests"))

local dir = util.shell_quote("spec/tracebacks/"..example)
local function assert_test(test, expected_content)
local dir = util.shell_quote("spec/tracebacks/"..test)
local ok, _, output_content, err_content =
util.outputs_of_execute("./pt-lua "..dir.."/main.lua")
assert(not ok, output_content)
Expand Down
Loading