UI option to turn on backtrace (RUST_BACKTRACE=1) --- off by default; (and also embed it in shared urls)

[respeccing]

UPDATE: the following is old info, jump to updated updated2 updated3

set RUST_BACKTRACE=1 when Debug build and does not set it when StableRelease build.

This addresses #119 although not entirely and I only say that because I imagine that a new UI
button would be desirable instead of it currently depending on the Mode(Debug/StableRelease) one.

Currently, if the Mode is set to Debug then the (dash)environmnet var RUST_BACKTRACE=1 is set both for when running rustc (in case the compiler itself panics) and when running the output binary (./out). But when Mode is set to StableRelease, it doesn't set this env. var.

Unless I'm missing something, this should work! as I have tested it inside a virtualbox (Manjaro(based on Archlinux) VM)

In web.js I've set backtrace to 2 which means that it will be set according to Mode (as explained above), but if an UI button would be added in the future, this button can directly set backtrace to 1 to enable backtrace or to 0 to disable it. (2 is the "auto" version, so to speak)

Thank you.

EDIT: Just so it's visually clear what I meant by Mode(Debug/StableRelease) see the red arrow in this screenshot:

EDIT2: And by "Stable" I meant "Release"(in the UI there), for some reason in my mind Release mapped to Stable and I remembered it this way.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alexcrichton]

This was proposed before, but I had some reservations because the spew isn't necessarily always wanted

[respeccing]

Oh I see. Since Mode: Debug is selected by default(ie. whenever visiting the rust-playpen website) then it's

always

spewed, as you've said.

Here's what I propose:

1. A separate independent setting for backtrace(on/off) with the default of off as to not change the default expected behavior of rust-playpen thus far (imagine old urls(eg. those that link to play.rust-lang.org) expecting to see the program output without RUST_BACKTRACE being set). EDIT2: Note that this is intended to only affect Run and not the compile for ASM/LLVM IR/MIR !
2. This setting would perhaps best be buried in Settings below Asm Flavor:, rather than be in plain sight near Mode: Debug/Release for example.
3. The setting ought to get embedded into the url (just like version(stable/beta/nightly) does) so that one can give a link to play.rust-lang.org with this setting enabled.

What do you think?

EDIT: Note that I explicitly didn't touch compile.sh as I figured one would never want a backtrace when just compiling for those options (ASM/LLVM IR/MIR) but rather, one would want the backtrace only when Run happens.(and when Run happens it has it's own compile in evaluate.sh which it too is affected by the backtrace setting, as intended)

[respeccing]

I did step 1 and 2 from my previous comment.

I could do step 3 too, but this means the value for backtrace in the url would override the value in the localStorage(and then have it even be saved in the localStorage) and I'm unsure how much sense this makes ...
On the other hand, what if you want an url with backtrace turned on?

Maybe the localStorage backtrace should be replaced with the one stored in the url?(this means it would act like version(Stable|Beta|Nightly) aka the Channel in the screenshot)
This way we would get both functionalities but without the persistence of the backtrace setting for subsequent rust-playpen sessions.
But then, if it's not in localStorage does it make sense to have it under Settings? Since Settings imply they will be stored(just like others) for subsequent rust-playpen sessions(eg. next time you open browser and visit rust-playpen).
**UPDATE:**Nevermind! I forget that localStorage only gets updated when the setting is changed by keyup and onChange events! This means, we can have both url-specified backtrace setting and at the same time keep the localStorage setting(should a new session be opened in a new tab for example).

[jonas-schievink]

I think we can set it for ASM / IR / MIR as well, maybe even unconditionally, since it will only show a backtrace on ICEs, which is always useful.

[respeccing]

I think we can set it for ASM / IR / MIR as well, maybe even unconditionally, since it will only show a backtrace on ICEs, which is always useful.

I wouldn't mind setting it(if @alexcrichton would agree) whether unconditionally or depending on the current setting.

[respeccing]

Alright, I've made this now so that it's satisfactorily for me.
Ignore any previous comments because, supposedly, this one summarizes them all.

Now, backtrace is a setting in Settings and it's off by default; it is also carried over in shared urls (playground URL and shortened). It is also stored in localStorage (just like Asm Flavor is) and it plays nicely with the setting from the url in that it only overrides it for the current session(that opened the url) - so new sessions will still use the localStorage setting.

(ignore the eval=1 in the screenshot, I was just guessing around)

What's left to consider is:

1. whether to have the backtrace also for ASM/LLVM IR/MIR as suggested here and whether that should be unconditionally enabled or only enable it as per the backtrace setting.
2. whether the third option of backtrace=auto should be removed (auto means it turns it on when Mode: Debug, else it's off, eg. when Mode: Release) - but note that the default value for backtrace if off by default! (not auto)
3. any corrections/modifications/removals for what is done thus far.

Thoughts, ideas, feedback?
Thanks!

[alexcrichton]

This all sounds good to me, thanks @respeccing! I'd personally say that we should have this off by default even for the compiler because only compiler devs are really interested in the giant backtraces on ICEs, and I suspect most play users aren't compiler devs

[alexcrichton]

Could the caller of this script just set the environment variable for the subprocess? This seems like crazy bash syntax that only one person in the world will ever understand :(

[respeccing]

That makes much more sense, I'll explore how to do just that. Thanks.

[respeccing]

Oh I forgot, looks like execute() is calling playpen.execute() which doesn't allow setting any env. vars ... and this is why I had to set RUST_BACKTRACE within evaluate.sh depending on the incoming args (--backtrace)

EDIT: that is, the playpen executable(which is the one executing evaluate.sh) isn't passing any outer env. vars that would be set by subprocess.Popen() and playpen itself doesn't have a way to set any new vars, unless you'd want me to use the hostname (playpen --hostname=NAME) as the state of the backtrace(?) though, that is hacky...

What do you think?

[respeccing]

tl;dr: I'll pass --backtrace as first arg, and remove it with shift, and also simplify the if test (although I borrowed the code for if from the below --test and --color=always code) which results in simpler code!

---

Don't read this, but I'll leave it here anyway :)

I used a simpler syntax in this commit: a147805
But that had the limitation that --backtrace must always be the first passed arg! else it would break things badly(eg. delete another arg!) So, if someone would modify web.py not knowing --backtrace must be first arg, then things would break so badly...

The ugly bash syntax(actually, it's running under the dash shell - so there are no arrays for example) doesn't have the above limitation and it's making sure that when --backtrace is specified wherever in the args list it will be safely removed, UNLESS it's specified more than once, then that will break. But I don't see why it would ever be specified more than once, unless a mistake in web.py but still not as easy of a mistake to make than in the above case with --backtrace having to me first arg.

set -- ${*%$backtrace*}${*#*$backtrace}
This resets the $* args (eg. it rewrites them) to have --backtrace removed by concatenating the two halves of the current $*, eg.
if $* is initially somearg --test --backtrace --color=always "arg with space" moreargs here
and $# is thus 7
The set above will concat somearg --test with --color=always "arg with space" moreargs here and set the result as being the new $* (the args passed to evaluate.sh are thus modified/reset to this new value)
and still ensure the number of args is the same minus 1, even if args with spaces exist!
So it becomes: set -- somearg --test --color=always "arg with space" moreargs here
thus $* is now somearg --test --color=always "arg with space" moreargs here
$# is 6

Actually I forgot to test for args with spaces and it's clearly not working for that case($# is 9!)! eg. args with spaces are split into more than one arg. eg. the one argument "arg with space" becomes 3 args: "arg" "with" "space"
But I thought I did test it...

It doesn't seem to be a way to also remove --backtrace AND keep the args with spaces intact, and I am unsure if it's necessary to keep them intact that way... they are only needed for this line, currently: TERM=xterm rustc - -o ./out "$@" In other words, if rustc currently(or in the future!) accepts any args with spaces(not counting the filename which is stdin here), then it's a MUST that I have to keep the args intact! I'll investigate more...

Worst case, we could revert to that that commit, it's the simpler syntax(as you'd want) AND it keeps the args intact! Hmm... I could even just check for $1, expecting --backtrace to be the first, and only then remove it, and if it's not first, then it will cause rustc to fail because it will get --backtrace passed to it ... yes... this seems like a simpler way, an interestingly simple compromise between what I want and what you want:)

[respeccing]

Hmm, I just realized something... the caller(webpy) can actually call playpen with the shell(dash, not bash) and pass it the script, but the best part is that, this way it could also set the env var, like so:
playpen ppargshere -- /usr/bin/dash -c 'export RUST_BACKTRACE=1; evaluate.sh eargshere'
or
playpen ppargshere -- /usr/bin/dash -c 'RUST_BACKTRACE=1 evaluate.sh eargshere'

(this I mentioned like 1 day ago in another issue and I forgot about it completely, see "EDIT3" here for the source of this)

So the question is, should I change this PR to do this?! it seems much better than having to pass(then remove) the --backtrace arg!

EDIT: the answer to your question is thus yes

Could the caller of this script just set the environment variable for the subprocess?

EDIT2: I am doing it anyway haha... can't help it! (sorry? xD)

EDIT3: the only problem seems to be that I might lose the wholeness of args-with-spaces, unless I would add doublequotes around each but that might be problematic if those args themselves contain such doublequotes already(which in our case is unlikely to be so, ever). I'll see what I can do... EDIT4: hmm, escaping the doublequotes from args is a great idea! :)) EDIT5: actually this is treading on dangerous territory lol... it seems safer to just not use dash -c with all that escaping quotes/doublequotes just in case things might get away ... even though it's under playpen... Seems way safer as it currently is, with the scripts hmmm.... Because I have to escape both single quote and double quotes, and I'm unsure if it's as airtight as it seems... ok, let me think more, maybe I'm exaggerating it in my mind.
EDIT6: Hmm I can escape single quotes within singlequotes as '"'"' =)) then this might actually work after all!! EDIT7: this would also work '\\''. EDIT8: this would do? must read/test
EDIT9: ok, I can't sort out the quoting... whilst already within a quoting... that is, I have to escape some args and then escape the whole result... it's a like double escaping and I think it would leak... and yield a security-issue... or I'm missing something. In the above eargshere are the ones that need to be escaped, but as you can see I'm already within single quotes... and those eargshere could(in theory) contain args with spaces too and even backticks, and they cannot leak. (i'll think about it again)
EDIT10: nevermind, it works! it just didn't in my mind :))
EDIT11: just realized I don't have to double quote! just have to make sure it's one arg!

[respeccing]

Should I also change the internal values for backtrace that is: on and off (and auto) to 1, 0 (and 2) ? EDIT2: NOTE: I don't mind doing it! In fact, I think I'll do it anyway :D can always revert the commit later if desired. EDIT3: Oops, I forget that what's seen in the UI (on/off/auto) is what's actually used as value internally too. Will try to find a way... EDIT4: value= it is... (yep, i dno html)

Maybe this is desired to reduce the length of the url (since the code is limited to 5000 chars in url, apparently). And maybe while at it, I could change backtrace in url to just b.
(The UI will still show on/off/auto though.)

EDIT: Because once this is merged, changing it later, might imply the need to support the previous variants too, in case shared urls were created in the meantime (or just leave those urls with non-workable embeded backtrace setting)

[respeccing]

Should I keep the (url)variable name backtrace= ? I'm thinking for clarity. This affects the length of the url, for example:
http://127.0.0.1/?code=fn%20main%28%29%20{%20panic!%28%2211212%22%29%3B%20}&version=nightly&backtrace=1

I notice that &version=nightly is already long, but it's clear! So maybe I should keep the word backtrace in &backtrace=1 ? for clarity. Else, I could just use bt=1 or b=1 even, to keep the url as short as possible so that more code can get in, at the expense of clarity I guess(because some might wonder what is that b= in the url?!). If we later decide to change from backtrace= to b= then I imagine we'd have to support both variants for compatibility with earlier shared urls.

I would really like some feedback on this, whenever possible. Thx.

(edit: otherwise, all is done, in theory)

[alexcrichton]

Ah yeah I wouldn't worry about the length of the URL option, we can always change it in the future if we need to and otherewise it's probably the least of our concerns in URL length :)

[alexcrichton]

I wonder, can this and the above block share code for building up the arguments to the compiler?

[respeccing]

Done(and tested). Thanks for saying that! DRY is my friend ;-) (so to speak)

[respeccing]

A little summary of what has been done in this PR: (conciseness fail by the way, haha, sorry)
First, here's a screenshot(I followed a link and then I pressed Gist, then looked under Settings):

(in the above, the backtrace setting was not yet saved in localStorage because it was my first time visit(after clearing browser cache/stuff) and I didn't click to change it yet - only clicked to look at the setting))

The backtrace setting is off by default, but once you set it on (or auto which means if Mode: Debug then backtrace is on, otherwise it's off eg. for Mode: Release) it will be stored in your browser (local storage) and thus remembered on your subsequent visits to the website. Another such setting stored in the browser is Asm Flavor, so nothing new there ;)
Shared urls(via Shorten and Gist buttons) will include the setting for backtrace in their url (except for the Gist URL). Because of this, shared urls that you follow will override the localStorage setting, temporarily for that session, because of the &backtrace=val in the url. (the val can be 0, 1 or 2 for: off, on or auto, respectively). Unless you manually go and (re)set Backtrace yourself under Settings(top right wheel within the page), your previously-set (localStorage)value will remain as it were and you can see this by opening a new tab and visiting the website anew (or just removing the &backtrace=val from the current url and reload the page by pressing Enter, for example.) then look at it under Settings.

The backtrace setting does not affect rustfmt (the Format button) so if this one gets an ICE, you won't see it in the output. (I could implement this but would require creating an extra (wrapper) script, similar to evaluate.sh, for running only rustfmt - which I'd be happy to do, if this is an acceptable idea; the script is needed in order to be able to pass an arg to it(EDIT well not necessarily! see this), so it then can set the env. var(RUST_BACKTRACE=1), because rustfmt too is ran directly by playpen(which is ran by web.py) and playpen has no real way of passing outer env. vars, or setting them(well, aside from HOSTNAME I guess, which it accepts as arg.)there's this way)UPD: implemented this, differently(commits are way below there)

The backtrace setting does affect the following actions: Run(/Test), ASM, LLVM IR, MIR, Shorten, Gist UPD: and Format too now, and if Backtrace is set to auto, it affects Mode: Debug(turns on backtrace) and Mode: Release(turns off backtrace).
(I can remove this extra option of auto, before this PR is merged, if this seems extraneous or isn't wanted; it's really not a problem! I won't feel like I've wasted anything if it's removed, btw; just somebody say so, if it's so desired!)

[alexcrichton]

Using extra shell syntax to specify the RUST_BACKTRACE env var actually seems pretty reasonable to me, could that be implemented here as well?

[respeccing]

Really enjoyed this thus far. Don't hesitate to let me know if any changes are required or anything like that.
(meanwhile I'll recheck and see if I forgot something)

EDIT: looks like I wrongly assumed this would set the variable export A but only this works: export A= - fixing(in the next commit, below) ;-) (the bad part is that I remembered testing the former to work lol - fail)
eg.

$ (export ABC; set|grep ABC)
_=ABC
$ (export ABC=; set|grep ABC)
ABC=
_=ABC

[alexcrichton]

Perhaps this could just be if optimize == "0"?

[respeccing]

Done as requested ;-)

EDIT: under the hood:
Backtrace on:

running: nightly /usr/bin/dash ('-c', 'export RUST_BACKTRACE=1; /usr/local/bin/evaluate.sh -C opt-level=0 -g --color=always')

Backtrace off:

running: nightly /usr/local/bin/evaluate.sh ('-C', 'opt-level=0', '-g', '--color=always')

PS: Please don't ask me to git squash this PR - I like commit history :) besides, are you gonna let all this generic goodness get to waste? :Dactually I don't care, I'm happy anyway :D