Don't use "mostly debug runtime" with release configurations #4112

SeanTAllen · 2022-05-11T19:37:51Z

This removes the inclusion of PONY_ALWAYS_ASSERT when building and
adds the small changes needed to compile with that change.

We haven't been running with PONY_NDEBUG on for a long time as
PONY_ALWAYS_ASSERT "turns off" PONY_NDEBUG. This means that for the
most part, config=release has result in debug versions of the runtime.

This commit also rearranges our PR CI order so that config=debug comes
before config=release. If we do introduce a compiler error, it is better
to trigger an assertion than a crash. We stop running CI after the first error
so we could be left with a segfault that would be explained by an assertion.
The reordering so that debug is run before release assures that we will see
the most informative error messages.

The commit also fixes an assert in refer.c that had a side-effect. Compiling ponyc with
assertions off would lead to a compiler that segfaulted because a required ast_pop
wasn't being done.

The commit includes many small changes to handle variables that are only used for
assertions and lead to compilation errors.

ponylang-main · 2022-05-11T19:38:26Z

Hi @SeanTAllen,

The changelog - changed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 4112.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

SeanTAllen · 2022-05-11T19:39:56Z

src/libponyc/codegen/gencall.c

@@ -1019,6 +1019,7 @@ static LLVMTypeRef ffi_return_type(compile_t* c, reach_type_t* t,

  if(t->underlying == TK_TUPLETYPE)
  {
+    (void)intrinsic;


i'm not sure how we want to deal with these things that aren't in an #ifdef that exist only for asserts.

this is my "get it to compile solution". any ideas for a better approach?

SeanTAllen · 2022-05-11T19:40:43Z

src/libponyc/pass/expr.c

      ast_t* body = ast_childidx(parent, 6);
+      (void)body;
      pony_assert(ast == body);


This feels like it should be wrapped in an ifdef so we dont do this extra lookup BUT... eh, does it matter as it is in the compiler and the ifdef would end up being weird.

SeanTAllen · 2022-05-11T23:10:42Z

huh so something in code that wasn't previously being executed is segfaulting. that's fun. i could use help debugging if anyone has time.

SeanTAllen · 2022-05-12T11:19:30Z

This is the backtrace building the runner:

Building collections/persistent -> /home/sean/code/ponylang/ponyc-clean/packages/collections/persistent
Process 9203 stopped
* thread #1, name = 'ponyc', stop reason = signal SIGSEGV: invalid address (fault address: 0x13)
    frame #0: 0x0000000000709d00 ponyc`token_get_id
ponyc`token_get_id:
->  0x709d00 <+0>: movl   (%rdi), %eax
    0x709d02 <+2>: retq
    0x709d03:      nopw   %cs:(%rax,%rax)
    0x709d0d:      nopl   (%rax)
(lldb) bt
* thread #1, name = 'ponyc', stop reason = signal SIGSEGV: invalid address (fault address: 0x13)
  * frame #0: 0x0000000000709d00 ponyc`token_get_id
    frame #1: 0x000000000076e034 ponyc`frame_push + 36
    frame #2: 0x0000000000730a14 ponyc`ast_visit + 100
    frame #3: 0x0000000000730ab0 ponyc`ast_visit + 256
    frame #4: 0x0000000000730ab0 ponyc`ast_visit + 256
    frame #5: 0x0000000000730ab0 ponyc`ast_visit + 256
    frame #6: 0x0000000000730ab0 ponyc`ast_visit + 256
    frame #7: 0x0000000000730ab0 ponyc`ast_visit + 256
    frame #8: 0x0000000000730ab0 ponyc`ast_visit + 256
    frame #9: 0x0000000000730ab0 ponyc`ast_visit + 256
    frame #10: 0x0000000000730ab0 ponyc`ast_visit + 256
    frame #11: 0x0000000000730ab0 ponyc`ast_visit + 256
    frame #12: 0x0000000000730ab0 ponyc`ast_visit + 256
    frame #13: 0x0000000000730ab0 ponyc`ast_visit + 256
    frame #14: 0x0000000000730f5d ponyc`ast_passes + 941
    frame #15: 0x0000000000730ba2 ponyc`ast_passes_program + 18
    frame #16: 0x0000000000742528 ponyc`program_load + 120
    frame #17: 0x0000000000703bed ponyc`main + 781
    frame #18: 0x00007ffff7c4d0b3 libc.so.6`__libc_start_main + 243
    frame #19: 0x000000000070381e ponyc`_start + 46

there's no get_token_id in frame_push so Im a little confused.

ergl · 2022-05-12T13:25:08Z

@SeanTAllen There's a call to ast_id on frame.c:38, which calls token_get_id inside. It is guarded by pony_assert(ast != NULL), which is perhaps being removed by this change?

Edit: the same applies inside token_get_id, which has an assert inside.

SeanTAllen · 2022-05-12T13:58:07Z

@ergl "It is guarded by pony_assert(ast != NULL), which is perhaps being removed by this change?" <-- i dont understand what you mean by "perhaps being removed by this change". All this is doing is turning on PONY_NDEBUG and fixing compilation errors that result from that. So something related PONY_NDEBUG or fixing the compilation errors triggers and error and in debug, there's no assert.

There's no assert triggered, nor any segfault with config=debug.

SeanTAllen · 2022-05-12T14:13:44Z

So runner can be built if...

I make it so that assert is always on (but PONY_NDEBUG is as well) + turn off well_formed_message asserts (2 of them) + make token and ast have a frozen variable so that all the asserts work.

SeanTAllen · 2022-05-12T14:33:46Z

Update:

So runner can be built if...

I make it so that assert is always on
PONY_NDEBUG is also on
turn off well_formed_message asserts (2 of them)
turn off frozen token asserts
turn off frozen ast asserts

SeanTAllen · 2022-05-12T14:55:49Z

AND here we go...

a side-effect in an assert. o lord:

this is around line 705 in refer.c

  pony_assert(typeref == ast_pop(ast));

SeanTAllen · 2022-05-12T14:59:10Z

It appears Joe was the baddie 5 years ago. Looks like we've been running with always assert on in the compiler for a long long time.

e84506d

chalcolith · 2022-05-12T15:04:54Z

Nice catch.

This removes the inclusion of PONY_ALWAYS_ASSERT when building and adds the small changes needed to compile with that change. We haven't been running with PONY_NDEBUG on for a long time as PONY_ALWAYS_ASSERT "turns off" PONY_NDEBUG. This means that for the most part, config=release has result in debug versions of the runtime.

SeanTAllen · 2022-05-12T15:57:36Z

src/libponyc/pass/refer.c

@@ -702,7 +702,9 @@ static bool qualify_typeref(pass_opt_t* opt, ast_t* ast)
  ast_setdata(ast, (void*)def);
  ast_setid(ast, TK_TYPEREF);

-  pony_assert(typeref == ast_pop(ast));
+  ast_t* popped = ast_pop(ast);


i'm open to better names than "popped". i haven't spent any time understanding the context of what was popped as it was just tossed.

SeanTAllen · 2022-05-12T20:28:12Z

The first comment on this PR is what should be used as the commit message when squashing.

src/libponyc/pass/refer.c

Co-authored-by: Joe Eli McIlvain <joe.eli.mac@gmail.com>

SeanTAllen requested a review from a team May 11, 2022 19:38

SeanTAllen added the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label May 11, 2022

ponylang-main added the discuss during sync Should be discussed during an upcoming sync label May 11, 2022

SeanTAllen added do not merge This PR should not be merged at this time needs discussion Needs to be discussed further and removed discuss during sync Should be discussed during an upcoming sync labels May 11, 2022

ponylang-main added the discuss during sync Should be discussed during an upcoming sync label May 11, 2022

SeanTAllen force-pushed the always-assert-off branch from 46161dc to 479a63f Compare May 11, 2022 19:39

SeanTAllen commented May 11, 2022

View reviewed changes

SeanTAllen force-pushed the always-assert-off branch 3 times, most recently from 25e740a to 8204529 Compare May 11, 2022 23:06

SeanTAllen added 4 commits May 12, 2022 11:41

WIP

08de0ef

More

9e23b40

Closer

37431a2

SeanTAllen force-pushed the always-assert-off branch from 40a803f to 37431a2 Compare May 12, 2022 15:41

Reorder apple arm64 ci

a90e79f

SeanTAllen commented May 12, 2022

View reviewed changes

SeanTAllen marked this pull request as ready for review May 12, 2022 20:22

Add release notes

e4ed5ea

jemc reviewed May 13, 2022

View reviewed changes

src/libponyc/pass/refer.c Outdated Show resolved Hide resolved

jemc approved these changes May 13, 2022

View reviewed changes

Update src/libponyc/pass/refer.c

4b57026

Co-authored-by: Joe Eli McIlvain <joe.eli.mac@gmail.com>

SeanTAllen removed do not merge This PR should not be merged at this time needs discussion Needs to be discussed further labels May 13, 2022

SeanTAllen merged commit 4b50803 into main May 13, 2022

SeanTAllen deleted the always-assert-off branch May 13, 2022 18:14

ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label May 13, 2022

github-actions bot pushed a commit that referenced this pull request May 13, 2022

Update CHANGELOG for PR #4112

3a623e0

github-actions bot pushed a commit that referenced this pull request May 13, 2022

Updates release notes for PR #4112

80861c9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use "mostly debug runtime" with release configurations #4112

Don't use "mostly debug runtime" with release configurations #4112

SeanTAllen commented May 11, 2022 •

edited

Loading

ponylang-main commented May 11, 2022

SeanTAllen May 11, 2022

SeanTAllen May 11, 2022

SeanTAllen commented May 11, 2022

SeanTAllen commented May 12, 2022

ergl commented May 12, 2022 •

edited

Loading

SeanTAllen commented May 12, 2022

SeanTAllen commented May 12, 2022

SeanTAllen commented May 12, 2022

SeanTAllen commented May 12, 2022 •

edited

Loading

SeanTAllen commented May 12, 2022

chalcolith commented May 12, 2022

SeanTAllen May 12, 2022

SeanTAllen commented May 12, 2022

Don't use "mostly debug runtime" with release configurations #4112

Don't use "mostly debug runtime" with release configurations #4112

Conversation

SeanTAllen commented May 11, 2022 • edited Loading

ponylang-main commented May 11, 2022

SeanTAllen May 11, 2022

Choose a reason for hiding this comment

SeanTAllen May 11, 2022

Choose a reason for hiding this comment

SeanTAllen commented May 11, 2022

SeanTAllen commented May 12, 2022

ergl commented May 12, 2022 • edited Loading

SeanTAllen commented May 12, 2022

SeanTAllen commented May 12, 2022

SeanTAllen commented May 12, 2022

SeanTAllen commented May 12, 2022 • edited Loading

SeanTAllen commented May 12, 2022

chalcolith commented May 12, 2022

SeanTAllen May 12, 2022

Choose a reason for hiding this comment

SeanTAllen commented May 12, 2022

SeanTAllen commented May 11, 2022 •

edited

Loading

ergl commented May 12, 2022 •

edited

Loading

SeanTAllen commented May 12, 2022 •

edited

Loading