Closed Bug 1499907 Opened Last year Closed Last year

When fixing windows symbols dmd.py can generate invalid json

Categories

(Core :: DMD, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: erahm, Assigned: njn)

Details

Attachments

(1 file)

Our file processing in dmd.py has a few steps:

#1) open the dmd file
#2) fix the the stacks and dump to a tmp file
#3) replace the original file with the tmp file
#4) carry on with the dmd report

When writing to the tmp file we blindly assume the fixed stacks are properly escaped, so for example we can end up with something like:

>  "DbT": "#00: static void std::vector<int,std::allocator<int> >::_Resize<`lambda at z:\build\build\src\vs2017_15.8.4\VC\include\vector:1487:23'>(const unsigned __int64, class std::vector<int,std::allocator<int> >::resize::<unnamed-tag>) [vs2017_15.8.4/VC/include/vector:1441]"

where we have unescaped '\' characters. When we go to parse the JSON it then fails to parse due to an invalid escape sequence.
The quote in |extern "C"| was also a problem.
So, `fixSymbols` isn't JSON aware. Conceptually, it just takes a line like 
this:

 "B": "#00: REPLACE_ME"

and replaces that "REPLACE_ME" bit like this:

 "B": "#00: extern "C" foobar"

we need this instead:

 "B": "#00: extern \"C\" foobar"

The cleanest way to do this would be to add an optional parameter to
`fixSymbols` that requests the transformed part be JSON-escaped.

(The alternative would be to do some parsing in dmd.py to find the substring that needs JSON escaping, but that's pretty gross.)
erahm, does this fix things for you?

bholley, you might also like to try it.
Attachment #9026571 - Flags: review?(erahm)
Attachment #9026571 - Flags: feedback?(bobbyholley)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Trying this patch would probably take me about fifteen minutes of fiddling recreate the setup on windows where this happened to me. I'm willing to do that if necessary, but want confirmation that it's really needed. Please NI if that's the case.
Attachment #9026571 - Flags: feedback?(bobbyholley)
Comment on attachment 9026571 [details] [diff] [review]
Add a `jsonEscape` argument to `fixSymbols`

Review of attachment 9026571 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, adding the option to fixSymbols is definitely the cleanest approach. I don't have a windows setup to test against right now, but logically this looks like it'll do the right thing.
Attachment #9026571 - Flags: review?(erahm) → review+
(In reply to Bobby Holley (:bholley) from comment #4)
> Trying this patch would probably take me about fifteen minutes of fiddling
> recreate the setup on windows where this happened to me. I'm willing to do
> that if necessary, but want confirmation that it's really needed. Please NI
> if that's the case.

I'm going to land the patch. If you have problems with it later on, let me know.
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fee6e5895b63
Add a `jsonEscape` argument to `fixSymbols`. r=erahm
https://hg.mozilla.org/mozilla-central/rev/fee6e5895b63
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.