Closed Bug 427536 Opened 17 years ago Closed 12 years ago

gcc-dehydra: need dehydra "type" representations from treehydra trees

Categories

(Developer Infrastructure :: Source Code Analysis, defect)

x86
Linux
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: benjamin, Assigned: dmandelin)

References

Details

Attachments

(4 files, 1 obsolete file)

I'm combining dehydra and treehydra logic in static-checking.js, and I'd like to re-use the logic at http://hg.mozilla.org/mozilla-central/index.cgi/file/5cb5ebda3312/xpcom/static-checking.js#l41 in the finalizer analysis. So I think that what I want is a function which takes a "tree" and returns a dehydra type from it. Whether it's done purely in JS or somehow calls into dehydra_convertType doesn't really matter to me.
Attached patch Random work (deleted) — Splinter Review
I started hacking on this and got stuck. Here's my WIP which segfaults. I'm going to work on something else and hope that one of dmandelin/taras can do this much quicker than I could.
Given that combining these plugins statically causeses issues, I will be undoing the combination as planned and implementing the multiplexor plugin. Thus the correct approach is pure JS. Then you can break out your routines into library and invoke them from both tree and dehydra. I'm going to focus on writing the paper until Wednesday. So I won't do this immediately (dmandelin is welcome to).
(In reply to comment #1) > Created an attachment (id=314104) [details] > Random work > > I started hacking on this and got stuck. Here's my WIP which segfaults. I'm > going to work on something else and hope that one of dmandelin/taras can do > this much quicker than I could. So I guess it depends on when you want it. Do you want to wait for Taras' new hotness, or do want me to hack up something that works now (presumably taking less than a day).
Attached patch More WIP (deleted) — Splinter Review
This is my next WIP. David, I don't think that Taras' new hotness will actually fix the bug, so can I get you to take it? I can help provide testcases to compare the results of treehydra/dehydra to ensure that the behavior matches.
Assignee: nobody → dmandelin
Depends on: 427661
I've made a lot of progress here, mostly just hand-porting stuff from dehydra_types.c and dehydra.c along with GCC macros. Making things exactly identical gets kind of complicated, so I wanted to ask a few questions about the spec before going too much further. So it looks like your key req is being able to reuse static-checking.js. In that script, you access these fields for types: kind, isIncomplete, name, bases, attributes, members, type, isArray, typedef, isPointer, isReference For attributes: name, value For members: isFunction, type I see that you also access Dehydra vars a little bit, reading the isConstructor, fieldOf, and methodOf properties, and you do Dehydra statement locs, but I'm guessing that part needs to change somewhat more, and you'll take care of that yourself, or do you want me to do that part too? And, the main question is, does it meet your spec to make the Treehydra wrapper create objects that are identical to Dehydra if only the fields listed above are considered?
I would like isConstructor. I don't need .methodOf here, because I'm accessing the members from the type and not from statements. And fieldOf isn't applicable because it isn't exposed on type objects, only on statements. I also need .loc, even though I currently don't use it I will need it for proper error reporting. For the immediate term I only need the listed fields to match. I'd hopefully like to eventually have all fields match and have a unit test which checks that identical output is produced, but once the basic framework is in place I can probably fix up the straggling details... I'm still getting my head around all this treehydra stuff.
Just a status update on this. Code is in my personal hg repo. [1] It works on most of the small test cases, with "works" defined as equality on the fields listed in comment 5. It almost certainly does not work on Moz, because it doesn't do templates identically to Deydra (although it does something semi-reasonable). This is what's blocked on bug 427661. Taras is the expert on bug 427661, so I've left it for him so far, but I guess he might be kind of busy with his presentations. So if you want this done quickly, let us know so either Taras can reprioritize it or else I can work on it. [1] http://hg.mozilla.org/users/dmandelin_mozilla.com/index.cgi/dehydra-types/ Note that the test.js, run by test.py, has dependencies on my treehydra-analysis library, but the dehydra-types conversion code itself should not have any deps on files outside this repo.
Just pushed the WIP. I think it works well, minus templates. The patch only adds files, doesn't modify anything, so it shouldn't be harmful to anyone. Besides the C++ stuff, there are a couple more issues to think about before this is done: 1. Packaging the script files. I packaged it as 3 files: dehydra_types.js, the dehydra-types code itself; gcc_dehydra.js, GCC adapter functions created specifically to support this; and dtimport.js, some functions I stole from another repo of Treehydra code I'm working on. Going forward, I'm not totally opposed to putting all GCC adapter stuff in one big file, but I think other units of functionality, like dehydra types or ESP analysis, should be in their own files. I do think adapter stuff that goes beyond "core" (like adding props to GCC objects) should be off by itself until stable. 2. Testing. I have a patch that allows the C function dehydra_convertType to be called as a JS builtin function from Treehydra, and I successfully used it for testing. It would be a good way to test things, but that patch has its own issues (it requires modifications to the lazy object setup). An alternative is to run Treehydra and Dehydra separately, print results from each, and then in a third script eval the results and compare them. Besides being clunkier, that also requires extra work to make sure the list of types printed corresponds exactly. 3. If dehydra_convertType can be called from Treehydra as a JS builtin, we might even just replace this stuff with that. I recall there was offered some reason not to, but I forgot what it was.
Just a random remark: there is no FIXED_POINT_TYPE on mac.
Depends on: 429534
(In reply to comment #8) > Just pushed the WIP. I think it works well, minus templates. The patch only > adds files, doesn't modify anything, so it shouldn't be harmful to anyone. > Besides the C++ stuff, there are a couple more issues to think about before > this is done: > > 1. Packaging the script files. I packaged it as 3 files: dehydra_types.js, the > dehydra-types code itself; gcc_dehydra.js, GCC adapter functions created > specifically to support this; and dtimport.js, some functions I stole from > another repo of Treehydra code I'm working on. > > Going forward, I'm not totally opposed to putting all GCC adapter stuff in one > big file, but I think other units of functionality, like dehydra types or ESP > analysis, should be in their own files. I do think adapter stuff that goes > beyond "core" (like adding props to GCC objects) should be off by itself until > stable. > > 2. Testing. I have a patch that allows the C function dehydra_convertType to be > called as a JS builtin function from Treehydra, and I successfully used it for > testing. It would be a good way to test things, but that patch has its own > issues (it requires modifications to the lazy object setup). An alternative is > to run Treehydra and Dehydra separately, print results from each, and then in a > third script eval the results and compare them. Besides being clunkier, that > also requires extra work to make sure the list of types printed corresponds > exactly. I actually like this approach. Once we have the multiplexor it will be quite nice. I think this will work without the multiplexor too. A) Run Dehydra output the json to this.aux_base_name + ".dehydra", then have Treehydra read_file that. Instead of using string comparison, one would be doing a structurally_equal comparison from system.js
Blocks: 424416
Depends on: 431236
Just a heads up so we don't step on each other, I'm gonna try finish up the "TODOs" in the current code right now.
OK, all done with the TODOs. I took the liberty of pushing as almost all the changes are in the unstable dir. Array lengths, template attributes, and base class information should now be available. Actually, I'm not too sure what the template attributes code is all about, so if anyone has a test case for that, please try it out or send it to me. The main thing that is still not identical to Dehydra is values of template things that are produced by the GCC pretty-printer in Dehydra. For example, for an instantiated template C<int> with a method f, we get names: Dehydra: C<int> C<int>::f Treehydra dehydra_types.js C C::f I can fix this example if needed, but I'm not sure how far we would want to go down that road, because (1) the template information is all exposed to JS, which can do the pretty-printing itself when needed, (2) copying the whole GCC pretty-printer is a lot of work to do if we don't really need to, and (3) if we really want to dup GCC behavior, we can do that a lot faster by exposing type_as_string and decl_as_string to JS.
So: currently I cache type objects by name in a map, e.g.: let classMap = {}; function getClass(typeobj) { if (classMap.hasOwnAttribute(typeobj.name)) return classMap[typeobj.name]; // otherwise do expensive calculations for whether a class is a GC object, // finalizable, final, etc. and add the result to the classMap } This requires that the .name of a class be unique-enough that nsCOMPtr<nsIFoo> and nsCOMPtr<nsIBar> are unique, that OuterClass::InnerClass and OuterClass2::InnerClass are unique, and that ClassName and namespace::ClassName are unique. I'm perfectly happy with exposing type_as_string and decl_as_string to JS, but I'm open to other possibilities as well, even to the point of automatically translating the prettyprinting functions from C to JS ;-)
Attached patch Minor type fixups, rev. 1 (deleted) — Splinter Review
These are some minor type fixups. Next up, I'm encountering performance problems. I'm going to port this code to use lazy lookup instead of doing full early recursion.
Attachment #318806 - Flags: review?(dmandelin)
Comment on attachment 318806 [details] [diff] [review] Minor type fixups, rev. 1 Looks good. The revised version will fail on integer template arguments, but it would fail on them before anyway. You might want to stick in a case if (TREE_CODE(arg) == INTEGER_CST) { return TREE_INT_CST_LOW(arg);
Attachment #318806 - Flags: review?(dmandelin) → review+
(In reply to comment #13) > So: currently I cache type objects by name in a map, e.g.: > > let classMap = {}; > > function getClass(typeobj) > { > if (classMap.hasOwnAttribute(typeobj.name)) > return classMap[typeobj.name]; > > // otherwise do expensive calculations for whether a class is a GC object, > // finalizable, final, etc. and add the result to the classMap > } > > This requires that the .name of a class be unique-enough that nsCOMPtr<nsIFoo> > and nsCOMPtr<nsIBar> are unique, that OuterClass::InnerClass and > OuterClass2::InnerClass are unique, and that ClassName and namespace::ClassName > are unique. You can do this with the Map and FastInjectiveMap classes by writing custom equality/hashcode or key functions. But it seems like a good enough reason to me to extend gcc_print.js to handle these examples, so I'm gonna do that. And it's not hard, because the dehydra_types template code essentially shows how to do it.
Pushed. Yeah, the current class map is a holdover from porting code from oink-dehydra, and it's showing.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Oops, didn't mean to resolve this while there's still working ongoing.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Improved pretty-printing (obsolete) (deleted) — Splinter Review
Improved the pretty-printing per bsmedberg's examples for namespaces, containing classes, and template instantiations. My secret test infrastructure shows that the .name fields are identical to Dehydra's. One tricky thing is that this makes decl_name (pretty-printer for DECLs) and type_string (for TYPEs) interdependent: printing a DECL can print a type (e.g., class that contains a FIELD_DECL) and printing a TYPE can print a decl (e.g., namespace that contains a RECORD_TYPE). So I moved decl_name into gcc_print.js with the other pretty-print functions. Now, the prototype for GCCNode refers to decl_name because its toString may invoke the pretty-printing, which is perfectly logical. So I include gcc_print.js from treehydra.js. treehydra.js includes other files already, and this seems to work OK.
Note: this patch uses tree_vec_iterate but that function is only defined in dehydra_types.js... I presume you want to just move it up a level.
With that fixed, I have another error building mozilla with the outparam analysis: /builds/gcc-dehydra/dehydra/libs/gcc_print.js:45: JS Exception: unhandled type: FUNCTION_DECL :0: #0: Error("unhandled type: FUNCTION_DECL") /builds/gcc-dehydra/dehydra/libs/gcc_print.js:45: #1: type_string([object GCCNode]) /builds/gcc-dehydra/dehydra/libs/gcc_print.js:25: #2: type_string([object GCCNode]) /builds/gcc-dehydra/dehydra/libs/gcc_print.js:31: #3: type_string([object GCCNode]) /builds/gcc-dehydra/dehydra/libs/gcc_util.js:191: #4: rectify_function_decl([object GCCNode]) ../../../../src/xpcom/analysis/outparams.js:35: #5: process_tree([object GCCNode]) gmake[5]: *** [nsDiskCacheDeviceSQL.o] Error 1
Attached patch Revised patch (deleted) — Splinter Review
Should take care of the FUNCTION_DECL error. Also moved up tree_vec_iterate.
Attachment #318951 - Attachment is obsolete: true
Dehydra and treehydra are no longer maintained by Mozilla.
Status: REOPENED → RESOLVED
Closed: 16 years ago12 years ago
Resolution: --- → WONTFIX
Product: Core → Firefox Build System
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: