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

Untodoed evil hack in Backtrace.pm #6395

Closed
p6rt opened this issue Jul 17, 2017 · 14 comments
Closed

Untodoed evil hack in Backtrace.pm #6395

p6rt opened this issue Jul 17, 2017 · 14 comments

Comments

@p6rt
Copy link

p6rt commented Jul 17, 2017

Migrated from rt.perl.org#131757 (status was 'resolved')

Searchable as RT131757$

@p6rt
Copy link
Author

p6rt commented Jul 17, 2017

From @AlexDaniel

See this​:
https://github.com/rakudo/rakudo/blob/6c76ed0abe352316eb58283fa6ce6b8150fc6830/src/core/Backtrace.pm#L144

It goes like this​:

  # now *that's* an evil hack
  next if $file.ends-with('BOOTSTRAP.nqp')
  || $file.ends-with('QRegex.nqp')
  || $file.ends-with('Perl6/Ops.nqp');

Creating a ticket so that it does not fall through the cracks.

@p6rt
Copy link
Author

p6rt commented Jul 17, 2017

From @toolforger

See this​:
https://github.com/rakudo/rakudo/blob/6c76ed0abe352316eb58283fa6ce6b8150fc6830/src/core/Backtrace.pm#L144

It goes like this​:

         \# now \*that's\* an evil hack
         next if $file\.ends\-with\('BOOTSTRAP\.nqp'\)
              || $file\.ends\-with\('QRegex\.nqp'\)
              || $file\.ends\-with\('Perl6/Ops\.nqp'\);

I think the whole concept of defining what's "interesting" in a
backtrace by looking at the file name is pretty evil​:

1) It cannot handle non-runtime code that one might want to filter.
2) It hardcodes the definition of what's interesting.
3) You cannot have runtime code that you *want* to be included in the
backtrace.
4) It adds a design constraint about what can go into which module of
the runtime.
5) The design constraint is nonobvious.
6) If somebody writes his own module in a different location but with a
matching name (e.g. they might be writing a wrapper, or an emulator),
then these files will be filtered as well.

Maybe it's smarter to have a function annotation (i.e. a trait, I
believe) for "don't include this in backtraces"; this would deal with
all problems except (2).
To address (2), the runtime could offer a setting that defines what
traits cause backtracking exclusion.

If a trait is not the way to go, one could deal at least with (5) by
adding a comment at the top of the filtered files​:

# Functions in this file will not show up in backtraces.
# See the filename filtering logic in Backtrace​::AT-POS
# of rakudo/src/core/Backtrace.pm.

I think line 148 has the same kind of evilness (decide what to do
depending on the name of the source file), I just couldn't determine
what the body of the "if" statement is actually doing​:

if $file.ends-with('NQPHLL.nqp') || $file.ends-with('NQPHLL.moarvm') {

@p6rt
Copy link
Author

p6rt commented Jul 17, 2017

The RT System itself - Status changed from 'new' to 'open'

@p6rt
Copy link
Author

p6rt commented Jul 17, 2017

From @geekosaur

On Mon, Jul 17, 2017 at 2​:40 AM, Joachim Durchholz <jo@​durchholz.org> wrote​:

I think the whole concept of defining what's "interesting" in a backtrace

by looking at the file name is pretty evil​:

So does Perl 6, actually; there's some discussion in the spec, and possibly
in the docs for callframe. The problem is, in the setting most of that
machinery is needed before it can be defined.

--
brandon s allbery kf8nh sine nomine associates
allbery.b@​gmail.com ballbery@​sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@p6rt
Copy link
Author

p6rt commented Jul 17, 2017

From @toolforger

Am 17.07.2017 um 09​:32 schrieb Brandon Allbery​:

On Mon, Jul 17, 2017 at 2​:40 AM, Joachim Durchholz <jo@​durchholz.org
<mailto​:jo@​durchholz.org>> wrote​:

    I think the whole concept of defining what's "interesting" in
    a backtrace by looking at the file name is pretty evil&#8203;:

So does Perl 6, actually; there's some discussion in the spec, and
possibly in the docs for callframe.

Do you mean that the Perl6 spec considers this to be evil, too?

The problem is, in the setting most
of that machinery is needed before it can be defined.

Is my understanding correct​: That we're looking at a chicken-and-egg
problem in the implementation?

@p6rt
Copy link
Author

p6rt commented Jul 17, 2017

From @jnthn

On Sun, 16 Jul 2017 23​:49​:55 -0700, jo@​durchholz.org wrote​:

6) If somebody writes his own module in a different location but with
a
matching name (e.g. they might be writing a wrapper, or an emulator),
then these files will be filtered as well.

The filenames end in .nqp. A Perl 6 users will be hugely unlikely to create such files.

Maybe it's smarter to have a function annotation (i.e. a trait, I
believe) for "don't include this in backtraces"; this would deal with
all problems except (2).

The code in question is implemented in NQP, which doesn't have a generalized traits mechanism.

As to disabling the filtering mechanism, that's entirely possible today with --ll-exception. The code in question is producing backtraces that will contain things relevant to a Perl 6 user, using these heuristics to filter out implementation guts that will almost certainly always be a distraction. Is it beautiful code? No. It's been a pretty good heuristic to date, however.

@p6rt
Copy link
Author

p6rt commented Jul 17, 2017

From @geekosaur

On Mon, Jul 17, 2017 at 4​:20 AM, Joachim Durchholz <jo@​durchholz.org> wrote​:

Am 17.07.2017 um 09​:32 schrieb Brandon Allbery​:

On Mon, Jul 17, 2017 at 2​:40 AM, Joachim Durchholz <jo@​durchholz.org
<mailto​:jo@​durchholz.org>> wrote​:

    I think the whole concept of defining what's "interesting" in
    a backtrace by looking at the file name is pretty evil&#8203;:

So does Perl 6, actually; there's some discussion in the spec, and
possibly in the docs for callframe.

Do you mean that the Perl6 spec considers this to be evil, too?

Yes. This is an acknowledged hack to provide the functionality when it
can't currently be done "right", and the ticket is a reminder aside from
the comment in the source that a proper solution is needed.

The problem is, in the setting most

of that machinery is needed before it can be defined.

Is my understanding correct​: That we're looking at a chicken-and-egg
problem in the implementation?

That's my read, but jnthn's observation that this lives in NQP-land also
complicates things.

(This is not the first chicken-and-egg problem we've had. The setting needs
booleans before perl 6 enums have been defined, so there's a BOOTEnum
thing. At one point it started leaking out, and because it's not a proper
perl 6 class but raw NQP it caused problems.)

--
brandon s allbery kf8nh sine nomine associates
allbery.b@​gmail.com ballbery@​sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@p6rt
Copy link
Author

p6rt commented Jul 17, 2017

From @toolforger

Am 17.07.2017 um 11​:34 schrieb jnthn@​jnthn.net via RT​:

The code in question is implemented in NQP, which doesn't have a generalized traits mechanism.

Ahh, may bad, this being NQP files flew right by me and I didn't see it.

I retract the traits idea - adding that just for this issue might be a
bit overblown, and I'm not deep enough into NQP to make any judgement
calls anyway.

I'd still like to see the dependency documented in the filtered files.
People will be more confident hacking the code if they see the relevant
ramifications documented.

@p6rt
Copy link
Author

p6rt commented Jul 17, 2017

From @AlexDaniel

I don't think this ticket needs so much discussion… It's basically about implementing the damn thing properly (and coming up with a way to do it along the way).

On 2017-07-16 21​:17​:22, alex.jakimenko@​gmail.com wrote​:

See this​:
https://github.com/rakudo/rakudo/blob/6c76ed0abe352316eb58283fa6ce6b8150fc6830/src/core/Backtrace.pm#L144

It goes like this​:

# now *that's* an evil hack
next if $file.ends-with('BOOTSTRAP.nqp')
|| $file.ends-with('QRegex.nqp')
|| $file.ends-with('Perl6/Ops.nqp');

Creating a ticket so that it does not fall through the cracks.

@p6rt
Copy link
Author

p6rt commented Jul 17, 2017

From @zoffixznet

On Mon, 17 Jul 2017 08​:33​:12 -0700, alex.jakimenko@​gmail.com wrote​:

I don't think this ticket needs so much discussion… It's basically
about
implementing the damn thing properly (and coming up with a way to do
it along
the way).

What problems does the current implementation have? What problem is the ticket wishes to be solved?

@p6rt
Copy link
Author

p6rt commented Jul 17, 2017

From @AlexDaniel

I think the purpose of the ticket was stated clearly in the first message.

Feel free to change the comment in the source code to “This is a hack but there's no point to improve it” and close the ticket.

To me it felt like the intention was to improve this part later, but no TODO comment was left, so we have this ticket. If I am being wrong, then yeah.

On 2017-07-17 09​:18​:29, cpan@​zoffix.com wrote​:

On Mon, 17 Jul 2017 08​:33​:12 -0700, alex.jakimenko@​gmail.com wrote​:

I don't think this ticket needs so much discussion… It's basically
about
implementing the damn thing properly (and coming up with a way to do
it along
the way).

What problems does the current implementation have? What problem is
the ticket wishes to be solved?

@p6rt
Copy link
Author

p6rt commented Jul 17, 2017

From @zoffixznet

On Mon, 17 Jul 2017 09​:25​:26 -0700, alex.jakimenko@​gmail.com wrote​:

I think the purpose of the ticket was stated clearly in the first message.

Feel free to change the comment in the source code to “This is a hack but
there's no point to improve it” and close the ticket.

To me it felt like the intention was to improve this part later, but no TODO
comment was left, so we have this ticket. If I am being wrong, then yeah.

On 2017-07-17 09​:18​:29, cpan@​zoffix.com wrote​:

On Mon, 17 Jul 2017 08​:33​:12 -0700, alex.jakimenko@​gmail.com wrote​:

I don't think this ticket needs so much discussion… It's basically
about
implementing the damn thing properly (and coming up with a way to do
it along
the way).

What problems does the current implementation have? What problem is
the ticket wishes to be solved?

We peek at the filename in many places in Backtrace class. The "evil hack"
comment was added when the full pathname was examined in the past and never
changed when code was changed. Lastly, I'm unware of any issues the current
code is causing or is predicted to be causing, so I removed[^1] the comment
and going to close the ticket.

[1] rakudo/rakudo@91be8bc1ca

@p6rt
Copy link
Author

p6rt commented Jul 17, 2017

@zoffixznet - Status changed from 'open' to 'resolved'

@p6rt p6rt closed this as completed Jul 17, 2017
@p6rt
Copy link
Author

p6rt commented Jul 17, 2017

From @geekosaur

On Mon, Jul 17, 2017 at 2​:49 AM, Joachim Durchholz via RT <
perl6-bugs-followup@​perl.org> wrote​:

1) It cannot handle non-runtime code that one might want to filter.
2) It hardcodes the definition of what's interesting.
3) You cannot have runtime code that you *want* to be included in the
backtrace.
4) It adds a design constraint about what can go into which module of
the runtime.
5) The design constraint is nonobvious.
6) If somebody writes his own module in a different location but with a
matching name (e.g. they might be writing a wrapper, or an emulator),
then these files will be filtered as well.

7. When Perl 6 gets used on the web, someone *will* find a way to abuse
this (e.g. to obscure how they broke into something).

--
brandon s allbery kf8nh sine nomine associates
allbery.b@​gmail.com ballbery@​sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant