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)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: benjamin, Assigned: dmandelin)
References
Details
Attachments
(4 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
dmandelin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•17 years ago
|
||
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.
Comment 2•17 years ago
|
||
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).
Assignee | ||
Comment 3•17 years ago
|
||
(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).
Reporter | ||
Comment 4•17 years ago
|
||
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.
Reporter | ||
Updated•17 years ago
|
Assignee: nobody → dmandelin
Assignee | ||
Comment 5•17 years ago
|
||
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?
Reporter | ||
Comment 6•17 years ago
|
||
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.
Assignee | ||
Comment 7•17 years ago
|
||
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.
Assignee | ||
Comment 8•17 years ago
|
||
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.
Comment 9•17 years ago
|
||
Just a random remark: there is no FIXED_POINT_TYPE on mac.
Comment 10•16 years ago
|
||
(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
Assignee | ||
Comment 11•16 years ago
|
||
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.
Assignee | ||
Comment 12•16 years ago
|
||
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.
Reporter | ||
Comment 13•16 years ago
|
||
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 ;-)
Reporter | ||
Comment 14•16 years ago
|
||
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)
Assignee | ||
Comment 15•16 years ago
|
||
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+
Assignee | ||
Comment 16•16 years ago
|
||
(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.
Reporter | ||
Comment 17•16 years ago
|
||
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
Reporter | ||
Comment 18•16 years ago
|
||
Oops, didn't mean to resolve this while there's still working ongoing.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 19•16 years ago
|
||
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.
Reporter | ||
Comment 20•16 years ago
|
||
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.
Reporter | ||
Comment 21•16 years ago
|
||
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
Assignee | ||
Comment 22•16 years ago
|
||
Should take care of the FUNCTION_DECL error. Also moved up tree_vec_iterate.
Assignee | ||
Updated•16 years ago
|
Attachment #318951 -
Attachment is obsolete: true
Comment 23•12 years ago
|
||
Dehydra and treehydra are no longer maintained by Mozilla.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 12 years ago
Resolution: --- → WONTFIX
Updated•7 years ago
|
Product: Core → Firefox Build System
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•