| | <<<Back 1 day (to 2018/09/02) | 20180903 |
| avih | tor8: you ok with taking a PR for test262 harness+launcher ? | 09:52.02 |
| | s/taking/considering/ | 09:52.44 |
| tor8 | avih: yeah, some automated test suite would be useful in the long run. | 10:01.39 |
| avih | tor8: k, i have a useful one. will send a PR soon. | 10:01.58 |
| tor8 | sebras: mvrhel (for the logs): In the "Fix string handling" commit -- jsut us fz_asprintf instead. | 10:06.43 |
| | that should be "just use" | 10:06.51 |
| | fz_set_icc_bgr is still a cryptic name. maybe there's a nicer way to accomplish the same, involving the constructor used to create the colorspace instead of creating it and then setting the flag. | 10:07.42 |
| sebras | oh! we have fz_asprintf()? gorram. | 10:08.16 |
| | tor8: what do you want to happend when fz_new_output_with_path() is called with filename == NULL? | 10:17.12 |
| | I accidentally stumbled upon this when trying to draw to gproof without setting the -o flag. | 10:17.26 |
| | and got a segfault. | 10:17.30 |
| | should we defer the NULL filename to fopen() and see what it does, or should we take action earlier and throw? | 10:17.57 |
| tor8 | sebras: a segfault would be appropriate... don't call functions that expect a value with NULL pointers. | 10:18.07 |
| | passing NULL to fopen also segfaults, IIRC | 10:18.20 |
| sebras | nope. | 10:18.27 |
| | if I check filename in fz_new_output_with_path() I get: | 10:18.44 |
| | error: cannot open file '(null)': Bad address | 10:18.47 |
| | so essentially it is only a question about whose error message we prefer. | 10:19.08 |
| tor8 | well, I guess they've made it more robust since I last passed NULL to fopen (back in the 90's) | 10:19.16 |
| | adding a bazillion paranoid NULL checks to guard against misuse of our API just makes the code needlessly heavy and complicated, IM(A)O | 10:20.50 |
| | it's one thing where we expect or want to handle a 'missing' object | 10:21.09 |
| | another where it makes zero sense to pass NULL | 10:21.17 |
| sebras | ok, in that case I'll add the check to the top of fz_save_gproof(). | 10:22.22 |
| tor8 | sebras: you mean to 'mudraw' so we don't call fz_save_gproof with NULL right? | 10:23.07 |
| sebras | we have an equivalent NULL check in fz_open_document(). | 10:23.42 |
| | pushing these checks too far up means we need to replicate them in the JNI code. | 10:24.27 |
| tor8 | sebras: that paranoia check is down to robin | 10:25.00 |
| | sebras: which, IMO, is the correct place to put them. java doesn't have a "non-null" object reference type, sadly | 10:25.30 |
| | we should be throwing a NullPointerException or IllegalArgument exception in the JNI code | 10:25.43 |
| sebras | ok. | 10:26.50 |
| tor8 | anyway, taht's my opinion. you and robin may think otherwise, and I won't object too strenously to some paranoid NULL checks. | 10:27.21 |
| | just don't go adding them to every goddamn function :) | 10:27.33 |
| sebras | I prefer the checks to crashes, so I might. ;) | 10:28.04 |
| tor8 | fix the call sites then :) | 10:28.18 |
| | things like mudraw.c | 10:28.24 |
| | or do you want to take a cue from the Eiffel language and add an assert-like macro "require()" | 10:29.20 |
| | which would be like assert but throw an exception instead of abort() | 10:29.36 |
| | require(filename != NULL) | 10:29.48 |
| | require(ctx, filename != NULL) would be more like it | 10:30.01 |
| sebras | mmm, well. now that I'm looking I notice that when output in mudraw is NULL we use fz_stdout for SVG. | 10:30.31 |
| | we could have done that in this case too. | 10:31.06 |
| tor8 | mudraw is not a shining paragon of good API design... | 10:31.21 |
| | sebras: we can detect NULL, but we can't detect other invalid pointers. where do you want to draw the line? | 10:32.53 |
| sebras | /* GPROOF files are saved direct. Do not open "output". */ | 10:33.08 |
| tor8 | someone could just as easily be passing memory that's been freed or uninitialized | 10:33.11 |
| sebras | /* SVG files are always opened for each page. Do not open "output". */ | 10:33.18 |
| tor8 | mudraw's file handling has driven me insane more than once... | 10:33.57 |
| sebras | tor8: I updated my branch, better? | 10:39.28 |
| | also if we want to compile with gproof enabled we should probably fix the tmpnam() warning. | 10:41.38 |
| | I did the ugliest hack I could think of at the top of the branch. | 10:41.52 |
| | mkstemp() returns an open fd, which is likely the better API, but VS seems to have tmpnam_s() which I assuem operates on an external buffer. | 10:42.36 |
| | so tmpnam_s() still has problem with atomicity. :( | 10:42.56 |
| tor8 | yeah, the problem is the code that assumes we can create a good tempfile name by string only :( | 10:53.50 |
| sebras | tor8: personally I'd really like to see us use popen() or something. | 10:54.25 |
| | tor8: so we wouldn't need a tempfile. | 10:54.35 |
| tor8 | sebras: problem is, I think, making the code work on windows too | 10:54.45 |
| | sebras: you have a #ifndef USE_GS_API for the #include <stdlib.h> to get system() | 10:55.22 |
| sebras | tor8: yes. | 10:55.31 |
| tor8 | but you don't ifdef the #include <assert.h> which is only used in the other branch | 10:55.35 |
| sebras | tor8: add #else or remove the #ifdef altogether? I can be convinced either way. | 10:56.05 |
| tor8 | I'd just remove the ifdef | 10:56.27 |
| | bad enough having ifdefs in one place, needing them to be in sync in separate locations isn't very appealing | 10:57.19 |
| | you can fz_asprintf in the tmpnam hack | 10:58.04 |
| sebras | https://msdn.microsoft.com/en-us/library/96ayss4b.aspx seems like VS has popen, but perhaps it is not present in VS2005. | 10:58.09 |
| | I haven't really bothered with that commit yet. | 10:58.29 |
| tor8 | instead of fz_tempfilename we should be using something like fz_output *fz_new_temp_file() | 10:59.34 |
| | but that means changing the calling API to gproof | 10:59.44 |
| sebras | tor8: I'm not sure that would work when we are suppling the tempfilename to the gsbinary which we run via system()... | 11:00.07 |
| tor8 | sebras: exactly. so it's not a 'temp' file per se. | 11:00.28 |
| sebras | tor8: actually VS2005 appears to have popen(). | 11:01.09 |
| tor8 | I'd suggest using ("%s.gprf", input_path) and check that we don't overwrite it or something like that instead | 11:01.14 |
| | but I wasn't involved in the gproof design, all that happened when I was on vacation | 11:01.32 |
| | sebras: good luck getting gs to use that, though | 11:01.51 |
| sebras | tor8: isn't this what the cluster does when it takes the md5sum of its output? | 11:02.26 |
| tor8 | sebras: use popen? | 11:02.49 |
| sebras | tor8: no, but its output is sent to stdout. so I'm thinking perhaps we could use popen() to read it. | 11:15.19 |
| | tor8: today gs outputs to a tempfilename that we open and parse later on. | 11:15.53 |
| | tor8: because gs is capable of outputting to md5sum so it ought to be possible to output in such a way that popen() can pick it up. | 11:16.37 |
| tor8 | sebras: right, you mean popen to read the stdout from another process | 11:17.38 |
| | gproof document type might need to be able to seek though (no idea, but that's a potential show-stopper) | 11:18.13 |
| sebras | it has a header and a table of file offsets to the bitmap for each page at the beginning it seems. | 11:20.35 |
| | this info is all cached immediately after calling gs. | 11:22.07 |
| tor8 | sebras: plenty of seeks in gprf-doc.c | 11:22.27 |
| sebras | tor8: that doesn't mean that they have to be there. | 11:23.20 |
| | some of them come across as fz_skip_bytes(). | 11:23.59 |
| | I think the most problematic thing would be that we need to cache the entire thing in memory to allow random pages to be rendered in rendom order. | 11:25.07 |
| | tor8: btw, I attempted to use my locally built gs to give me gproof-output, but it just errored out. perhaps it need to build in some special way. | 11:28.18 |
| | mvrhel (for the logs): where does default_cmyk.icc and srgb.icc come from when I attempt to run build/debug/mutool draw -F gproof -o document.gproof document.pdf && build/debug/mutool draw -F png -o document-%02d.png ./document.gproof ? I can see that mutool attempts to run system("gswin32c.exe -sDEVICE=gprf -sOutputICCProfile=default_cmyk.icc -sPostRenderProfile=srgb.icc -dSAFER -dFitPage -o \"\" | 12:10.10 |
| | -dFirstPage=1 -dLastPage=1 -I%rom%Resource/Init/ -g421x595 \"document.pdf\"") | 12:10.16 |
| | mvrhel (for the logs): Running that manually caused a segfault in gs that I reported https://bugs.ghostscript.com/show_bug.cgi?id=699699 due to the -o "" argument which is not handled. | 12:10.45 |
| | mrvhel (for the logs): correcting that flag and running gswin32c.exe -sDEVICE=gprf -sOutputICCProfile=default_cmyk.icc -sPostRenderProfile=srgb.icc -dSAFER -dFitPage -o "document.gproof" -dFirstPage=1 -dLastPage=1 -I%rom%Resource/Init/ -g421x595 "document.pdf" causes the error "base/gsicc_manage.c:1840: gsicc_verify_device_profiles(): Post render profile not supported by device" | 12:12.18 |
| | removing "-sPostRenderProfile=srgb.icc" for that commandline allows for an output file to be produced, but then I can't render that using mupdf. I'm confused. I must be doing something wrong here. | 12:13.50 |
| avih | tor8: https://github.com/ccxvii/mujs/pull/73 | 12:43.48 |
| | some of the code might look a bit awkward, but personally i value greatly usability and usefullness of the output | 12:47.15 |
| tor8 | avih: you might be able to use "new Function(read(filename))" instead of making a function compile() | 12:52.11 |
| | that will execute with new variables placed in a local function, so won't pollute the global namespace for each test run | 12:52.49 |
| avih | tor8: doesn't work. the test code assigns to free variables and expects to see them as properties of the global object (in some tests) | 12:52.59 |
| | (and vice verse, e.g. var x=7 and then it expects to see global.x === 7) | 12:53.35 |
| | (using a function constructor was my first attempt) | 12:54.06 |
| tor8 | I have modelled the mujs shell functions after what Mozilla's JS shell has | 12:55.06 |
| avih | not sure i understand the context... | 12:55.29 |
| tor8 | oh... they use the browser non-standard 'global' context? ick! | 12:55.51 |
| | sorry, s/context/name/ | 12:56.05 |
| | avih: adding new functions like compile() and cli_arguments | 12:56.22 |
| avih | not sure. "global" is mostly used in node.js envs. i didn't mean literally "global", just.. the global object (in the tests) | 12:56.54 |
| tor8 | avih: have you got a specific test that fails on this? | 12:57.15 |
| avih | (i.e. Function("return this")() ) | 12:57.28 |
| tor8 | right. abusing the 'this' binding in non-strict code to access the 'global' object. | 12:57.56 |
| avih | tor8: yes, i don't recall which though, it was one of the earlier iterations | 12:57.58 |
| tor8 | mozilla's js shell has 'scriptArgs' | 12:58.41 |
| avih | ah, yeah, i didn't want to use "arguments" but scriptArgs sounds better. I can change it, sure | 12:59.10 |
| tor8 | "arguments" would fail spectacularly, I think (it's a reserved word, with special semantics) | 13:00.11 |
| avih | exactly | 13:00.19 |
| | never even tried it :) | 13:00.24 |
| tor8 | might be worth cloning the js24 shell command line option behaviour too | 13:00.43 |
| avih | 24? isn't the most recent stable 38? (iirc...) | 13:01.07 |
| tor8 | it's what I have on my ancient debian install | 13:01.27 |
| avih | right. iirc after 38 they stopped pretending other can use it, so it got stuck at 38 | 13:01.51 |
| tor8 | used to be mozjs or something like that | 13:02.15 |
| avih | yeah | 13:02.21 |
| | it could emulate node.js, which is more "live". that'd be process.argv[] | 13:03.34 |
| | (which could then support also process.env, though not required for now) | 13:04.11 |
| | tor8: did you run the entire suite with this? | 13:18.48 |
| | hmm.. actually, i can remove the crashing tests from KNOWN_BAD . it was required when it was looping the tests in js, but now that every test executes independently it can crash. there are only 3 such prematurely-aborting tests | 13:20.32 |
| | on my system it takes about 2 mins. it takes about 1 min if i don't load harness/testIntl.js (all the Intl tests fail anyway, but for completion it should load and test it) | 13:24.19 |
| tor8 | avih: there's a getopt-using patch on tor/master that adds scriptArgs | 13:31.04 |
| | and -i and -s command line options | 13:31.17 |
| avih | tor8: sure, i don't mind how they get in. feel free to push anything else and i'll align the test262 files | 13:31.40 |
| | (push to master, that is. i prefer not to base a PR on top of your private branch) | 13:32.41 |
| tor8 | avih: I'm seeing a lot of "[run] Test262 Error: Test case returned non-true value!" | 13:45.48 |
| avih | tor8: yes, that's one way it reports an error. the test/suite is responsible for all the strings there except "[run]" | 13:46.29 |
| | the [run]/[load] are the stage at which it failed. load == compile time. run == runtime | 13:47.14 |
| | as far as i can tell there's no other useful info which can be extracted on those cases | 13:48.23 |
| | the suite is also not 100% consistent in its reports. some tests report with $ERROR - which is creates a Test262 error instance with the string, other tests simply throw a string, etc | 13:50.07 |
| | there are quite a few math precision errors. those can be overridden using the -l option, e.g. to load a ./lower-precision.js file which looks like: | 13:52.18 |
| | (function(global){ | 13:52.19 |
| | var orig = global.getPrecision; | 13:52.19 |
| | global.getPrecision = function(num) { | 13:52.19 |
| | return orig(num) * 1e10; | 13:52.19 |
| | }; | 13:52.19 |
| | })(this) | 13:52.21 |
| | the original one at sta.js has some shady calculation of precision | 13:53.00 |
| | (harness/sta.js is pre-generated from harness/math_precision.js and others) | 13:55.20 |
| | the individual files use $INCLUDE as loader. might be better to replace it with our loaded and thus letting it load only files it needs, e.g. not loading the Intl thingy for every test (didn't try it though) | 13:57.09 |
| | overall though, i think most errors i've looked at look plausible. i.e. good reports | 13:59.30 |
| tor8 | most of the ones in ch07 seem to be down to mixed strict/non-strict code | 14:02.19 |
| avih | yes | 14:02.35 |
| tor8 | the ones with @onlyStrict directives | 14:02.36 |
| | and eval | 14:03.09 |
| avih | hmm.. didn't look too deep into those. does the directive imply something for the test env, which is not taken care of? | 14:03.09 |
| tor8 | the 'eval' directive isn't inheriting strictness from the surrounding function | 14:03.30 |
| avih | i.e. is there a test you think should pass but it fails? | 14:03.34 |
| tor8 | which is probably a bug | 14:03.39 |
| avih | (when checking how the test looks like, i find the -f option useful, it reports the full path so it can be copied easily to less, for instance) | 14:04.44 |
| | (otherwise, by default, it reports only the "normalized" path to make it more comparable between users, but it's less useful when actually working on fixing things) | 14:05.32 |
| | tor8: i can add an option to print the test source on failures, i would be trivial. should i? | 14:06.13 |
| | it* | 14:06.33 |
| tor8 | no, but the full path would be better :) | 14:07.39 |
| avih | tor8: just add -f to see the full path | 14:07.50 |
| | by default i preferred normalized paths | 14:08.15 |
| tor8 | with the fixed eval strictness, most are now down to octal escapes | 14:08.31 |
| | ah. | 14:08.37 |
| avih | (-f also shows full stack traces rather than trimming the loader out of the trace) | 14:09.16 |
| | so -f is "developer output" | 14:09.45 |
| tor8 | avih: thanks for doing this. I'm sure it will be helpful (and give me lots to do...) | 14:17.46 |
| | avih: I've pushed two new commits to origin/master | 14:18.00 |
| avih | there are definitely classes of errors which pop up. like the eval thingies, math precision, and others | 14:18.05 |
| tor8 | one for the scriptArgs and one for the eval fix | 14:18.07 |
| | yeah. I'm hopeful that once we fix one bug, lots of errors will go away in the same fix | 14:18.32 |
| avih | k, i'll rebase the PR accordingly | 14:18.34 |
| tor8 | avih: and I've added your github repo to my remotes, so you just need to poke me here and I can look without you needing to create github pull requests, etc. | 14:19.06 |
| avih | tor8: one class i noticed is the error name which it expects. seems like many times mujs throws a TypeError while it expects a more specific name | 14:19.36 |
| | tor8: no hassle at all to rebase the PR. i just force push my branch and the PR gets updates automatically | 14:20.11 |
| | updated* | 14:20.20 |
| | (that's how github PRs work) | 14:20.36 |
| | tor8: you don't include getopt.h ? | 14:23.22 |
| | hmm.. linux and freebsd say #include <unistd.h> is enough... | 14:24.31 |
| tor8 | getopt.h is not standard. I just include the short original public domain implementation at the top instead :) | 14:24.33 |
| avih | oh, missed that :) | 14:24.48 |
| tor8 | this way it works on windows too | 14:24.49 |
| | yeah, it's so short and sweet :) | 14:24.58 |
| avih | nice :) | 14:25.26 |
| | tor8: and compile(..)? | 14:25.45 |
| tor8 | not like the gnu getopt which is a bizarre monstrosity | 14:25.52 |
| avih | it is, yes | 14:26.07 |
| | iirc there are some cases where it's impossible to make it behave strictly posix. something with the args contradicting themselves | 14:26.35 |
| | (i knew the case, just don't recall it exactly) | 14:27.17 |
| moolc | tor8: gnu getopt has to handle --options, unlike getopt (which is POSIX and hence quite standard btw) | 14:28.36 |
| avih | tor8: so how should extra args be passed now? still -- foo bar ... ? | 14:28.43 |
| tor8 | we only do one script now (like js24) | 14:31.20 |
| | mujs [options] [script [scriptArgs*]] | 14:31.42 |
| avih | oh... so if there's more than one arg you assume 2+ are scriptArgs ? | 14:32.43 |
| tor8 | yes. | 14:32.49 |
| avih | k | 14:32.54 |
| tor8 | IMO a bit worse, but it matches what mozjs and nodejs do | 14:33.15 |
| avih | (yeah, after eating -s/-i) | 14:33.25 |
| | whatever. it works, it can load other files, doesn't really matters IMO | 14:33.42 |
| | tor8: fwiw, compile is also needed if one wants to implement a nicer require, so that it has the actual file name in stack traces | 14:37.12 |
| | tor8: updated. re-fetch. | 14:47.08 |
| tor8 | avih: hm, should compile() work like load() from a filename? | 15:08.43 |
| sebras | tor8: does "make one" in mujs build for you? for me it complains about lacking floor/atan/etc from libm.so | 15:09.12 |
| tor8 | sebras: it does not, but 'one' is not an explicit target in the makefile (anymore) | 15:09.51 |
| | when you run 'make one' it's using the implicit rules %: %.o and %.o: %.c | 15:10.20 |
| | or rather %: %.c | 15:10.29 |
| | sebras: we always build with one.c (so I don't break it...) | 15:11.07 |
| sebras | ok, I was just looking through the Makefile changes to see what had happened. | 15:19.48 |
| | btw, with gcc 8.2.0 I'm seeing large warnings about fallthroughs on jssstring.c:429 and 526, as well as regexp.c 113. | 15:20.43 |
| | those actually appear already with gcc 7.3.0 | 15:21.28 |
| tor8 | sebras: yeah, two of those are intentional (the comment says "fall through") | 15:22.28 |
| | the regexp.c calls die() which never returns (it longjmps out of there) | 15:22.40 |
| sebras | I know, I'm looking into how to make gcc ignore them. | 15:22.44 |
| tor8 | maybe spell it 'fallthrough' ? | 15:22.54 |
| | adding a break; after die() would probably work. | 15:23.07 |
| | and not be too offensive. | 15:23.12 |
| sebras | yeah, looks like it might be the spelling. | 15:25.01 |
| | mujs:sebras/master works for me | 15:31.29 |
| avih | tor8: no. compile(str [, as_filename]) | 15:38.28 |
| tor8 | avih: I mean, maybe we should make it be like load() ... i.e. compile(filename) | 15:39.40 |
| avih | tor8: it's trivial to create such compile of your own. it's meant to be low level. | 15:40.23 |
| | tor8: one thing it won't allow, for instance, is normalized/modified file names. it doesn't even require a file, but your version will not work without an actual file on the filesystem | 15:42.47 |
| tor8 | same limitations as 'load' | 15:45.07 |
| avih | tor8: not requiring an actual file on disk is a very powerful thing. e.g. for mpv i implemented require by compiling such file which i composed on the fly from the file on disk plus a wrapper | 15:45.29 |
| | yes, and i don't want that limitation :) | 15:45.41 |
| tor8 | fair enough. | 15:46.04 |
| | ah, looks like mozjs has added an 'evaluate' function which is like load but for source code strings | 15:46.38 |
| | like eval without the baggage | 15:46.58 |
| avih | the mozjs thingy is mostly a dead thing imo | 15:48.43 |
| | i think compile is very straight forward, and closely follows mujs public api. i don't care about its name, but i think its functionality has proved very useful more than once (for me) | 15:49.58 |
| | Forward 1 day (to 2018/09/04)>>> | |
Log of #mupdf at irc.freenode.net.