Closed Bug 1321014 Opened 3 years ago Closed 11 months ago



(Core :: JavaScript: GC, defect, P3)




Tracking Status
firefox64 --- fixed
firefox65 --- fixed


(Reporter: sfink, Assigned: sfink)


(Blocks 1 open bug)


(Keywords: triage-deferred)


(5 files, 1 obsolete file)

From bug 1277368.

At first glance, I don't see anything difficult about implementing this support. Right now, the hazard analysis uses things like __attribute__((tag("GC Pointer"))); this just means also recording __attribute__((annotate("moz_inherit_type_annotations_from_template_args"))) and then implementing support for it in the analysis. I was thinking it would be nicer if the plugin could implement this internally so that future type-specific annotations would be handled automatically, but it'd probably be a little harder to implement there (in weird C code using the gcc API), and it doesn't buy that much.

The tricky part is going to be figuring out what the types T and E are. I predict something awful like type string parsing. :(
Keywords: triage-deferred
Priority: -- → P3
I arbitrarily chose to use __attribute__((tag("foo"))) for attributes. But now it turns out that I want to use MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS so I can say that Maybe<JSObject*> is a GC pointer. So I'm switching to using __attribute__((annotate("foo"))) to match what mfbt/Attributes.h uses.

But note that I'm using a different style of string for the attributes. mfbt seems to use single-word strings like __attribute__((annotate("moz_inherit_type_annotations_from_template_args"))). I've been using short phrases, and kind of assuming I had the world to myself: __attribute__((annotate("Invalidated by GC"))). I don't know if that will cause any issues with the clang analyses?

Asking Nika for review in case she'd like me to stay in my own namespace. I would then look at both annotate(...) and tag(...), or perhaps switch to something like gcannotate(...).

There are other attributes that can be shared, eg at the moment I have JS_HAZ_CAN_RUN_SCRIPT which is fully redundant with MOZ_CAN_RUN_SCRIPT (I will discard mine before landing.)
Attachment #8996503 - Flags: review?(nika)
Assignee: nobody → sphink
Handle things like Maybe<JSObject*>: "T is a GC type" should imply "Maybe<T> is a GC type".

This also allows eg Result<JSObject*>.
Attachment #8996504 - Flags: review?(jcoppeard)
Nika - forgot to mention, but you can see it in the other patch: the MFBT macros are conditionally compiled, so I extracted out the one I wanted and enabled it for the hazard analysis as well. Come to think of it, I'm not sure why I don't just enable all of them for my analysis -- I ignore unrecognized attributes anyway. Opinions welcome.
No need for review on this patch. I will request review on the actual sixgill changes instead.
I tracked down a problem where I was getting attributes recorded for functions that did not have those attributes. It turns out that sixgill did not consider attributes during function comparison, which will now start to matter as I'm using attributes for more things.
Attachment #8996506 - Flags: review?(bhackett1024)
Oops, I uploaded the wrong patch here.
Attachment #8996506 - Attachment is obsolete: true
Attachment #8996506 - Flags: review?(bhackett1024)
Attachment #8996510 - Flags: review?(bhackett1024)
Comment on attachment 8996505 [details] [diff] [review]
Update sixgill to fix bug where functions could inherit attributes from functions identical except for the annotations, r=me

(sixgill update; see other attachment)
Attachment #8996505 - Flags: review+
*This* is the patch I meant to upload. The other sixgill patch here is mechanical.
Attachment #8996511 - Flags: review?(bhackett1024)
Comment on attachment 8996503 [details] [diff] [review]
Switch Tag to Annotate

Review of attachment 8996503 [details] [diff] [review]:

We originally decided to use namespaced attributes in case we ever wanted to pull in clang analyses from other vendors, and because they were hidden in Attributes.h.

I have no problem with you switching to share a namespace with the clang analysis, and you can use whatever names you want so long as they don't conflict with the clang-plugin ones unintentionally :-)
Attachment #8996503 - Flags: review?(nika) → review+
Attachment #8996510 - Flags: review?(bhackett1024) → review+
Attachment #8996511 - Flags: review?(bhackett1024) → review+
Comment on attachment 8996504 [details] [diff] [review]
Respect MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS for the purpose of identifying GC types and pointers

Review of attachment 8996504 [details] [diff] [review]:

::: js/src/devtools/rootAnalysis/computeGCTypes.js
@@ +166,5 @@
> +    //
> +    //   foo<T>::bar<U>
> +    //   foo<bar<T,U>>
> +    //
> +    const [_, params_str] = csu.match(/<(.*)>/);

Does this cope with nested templates?  Oh the comment mentions that.  Don't we have any nested templates with GC things in?  I think this needs  a followup to fix this.

::: js/src/devtools/rootAnalysis/t/hazards/source.cpp
@@ +97,5 @@
>      usecell(cell5);
> +    {
> +        // Templatized container that inherits attributes from Cell*, should
> +        // report a hazard.

Thanks for adding tests.
Attachment #8996504 - Flags: review?(jcoppeard) → review+
Pushed by
Switch Tag to Annotate, r=nika
Respect MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS for the purpose of identifying GC types and pointers, r=jonco
Backout by
Backed out changeset c7b32ffa822e on a CLOSED TREE
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Blocks: 1500247
Depends on: 1500640
The important patch here has not yet landed, and it appears that it is blocked on clang 7.
Depends on: 1498072
Resolution: FIXED → ---
Depends on: 1492663
No longer depends on: 1498072
Pushed by
Respect MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS for the purpose of identifying GC types and pointers, r=jonco
Closed: 11 months ago11 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.