zero_alloc new warning for unchecked functions by gretay-js · Pull Request #1302 · oxcaml/oxcaml

gretay-js · 2023-04-11T18:52:22Z

A function annotated with [@zero_alloc] may be optimized away completely by the middle end. As a result, the static allocation checker in the backend never gets to see and analyze this function, and cannot report a violation of the property, even if the function is allocating. From the user's point of view, the checker appears to be unsound or at least this behavior is misleading.

This PR implements a workaround: give a warning about [@zero_alloc] attributes that appear in the source but were not checked. It's not clear how the user can address the problem, other than removing the attribute. In some cases it may be possible to annotate the caller or mark the function as [@inline never]. For details see #1273

I added a new warning, rather than using Misplaced_attribute because the hint is different and we can also detect really-misplaced [@zero_alloc] attributes separately. The new warning is 199 to avoid conflicts with future changes upstream in warning numbers. This high number will make some operations on Warnings slightly expensive but I don't think it is significantly so. I also propose to number flambda-backend specific warnings from 199 down to avoid changing Warnings.last_warning_number every time.

It is not strictly necessary for the code for tracking the unchecked attributes to be in Builtin_attributes, next to the existing logic for Misplaced_attribute handling, because unchecked attrbutes are only needed in Lambda and the code mostly independent from Misplaced_attribute handling (except the use of Attribute_table and sorting).

ccasin · 2023-04-11T21:42:47Z

If I understand correctly, I think the current implementation will issue the new warning too often. In particular:

The bytecode compiler runs translattribute but not the mach stuff, so we probably never remove the attribute from the table and a warning is issued? Should we just ignore this attribute when compiling to bytecode?
If -zero-alloc-check is not passed, I think we won't run the check and so the attribute is never removed from the table. Do we want to be issuing a warning in this case?

lpw25 · 2023-04-12T07:51:55Z

Isn't this just worse than #1273? Why aren't we doing that one?

gretay-js · 2023-04-12T08:00:03Z

Yes, it would be better to keep the annotated functions from being removed, but it is not entirely clear how to do it, as mentioned in #1273, and so in the meantime we can have the warning as a workaround.

gretay-js · 2023-04-12T09:00:10Z

The bytecode compiler runs translattribute but not the mach stuff, so we probably never remove the attribute from the table and a warning is issued? Should we just ignore this attribute when compiling to bytecode?

If -zero-alloc-check is not passed, I think we won't run the check and so the attribute is never removed from the table. Do we want to be issuing a warning in this case?

oh, that was a bug. fixed in 3f8f14e, thank you!

had to rebase this PR on top of #1294 to be able to refer to zero_alloc_check flag from Translattribute.

Support for registering and marking property attributes.

The condition should be more general and check for the flag that correponds to the property attribute but for now we only have one.

ccasin

Looks good now!

xclerc

(I think there is a minor problem:
the warning will not trigger if the
check is requested through the
[@@@zero_alloc check] global
annotation rather than through
the local [@zero_alloc] one.)

gretay-js · 2023-04-13T09:15:35Z

(I think there is a minor problem: the warning will not trigger if the check is requested through the [@@@zero_alloc check] global annotation rather than through the local [@zero_alloc] one.)

What do you mean? [@@@zero_alloc check] turns the computation of summaries on, but does not assert that functions are not allocating. It is meant to be just an alternative to -zero-alloc-check compiler flag. By itself, the flag does not trigger any warnings. If a function is annotated with [@zero_alloc] and the check is on, then there will be a warning.

gretay-js · 2023-04-13T09:21:46Z

Were you referring to [@@@zero_alloc all] from #1296 ?

xclerc · 2023-04-13T09:26:46Z

Oh, indeed I was confused I somehow
thought the annotation was asking for
functions to be zero-alloc.

a7d005a flambda-backend: Lazy deserialization of cmi files (oxcaml#1322) aa83fa3 flambda-backend: Reinstate previous API for Env.lookup_value (oxcaml#1323) e4007a4 flambda-backend: Lazy substitution into value_declaration (oxcaml#1320) 634b607 flambda-backend: Merge Types.* and Subst.Lazy.* types (oxcaml#1312) cf82708 flambda-backend: Bump magic numbers for 4.14.1-7 (oxcaml#1317) 6470400 flambda-backend: zero_alloc attribute payload "assert all" and "ignore" (oxcaml#1296) bba5248 flambda-backend: Teach `ocamldep` about all the language extensions (oxcaml#1303) 33e97b0 flambda-backend: Change Includemod to work on lazy modtypes (oxcaml#1228) 16e5002 flambda-backend: zero_alloc new warning for unchecked functions (oxcaml#1302) 36b4626 flambda-backend: Attribute [@@@zero_alloc check] to turn the check on (oxcaml#1294) 3b524c6 flambda-backend: Cmm.value_kind cleanup (oxcaml#1091) ec99505 flambda-backend: Fix failure of `check_all_arches` on RISCV (oxcaml#1300) 450bc58 flambda-backend: Backend changes for multiple returns (oxcaml#1268) 84a7a26 flambda-backend: Static check for zero_alloc: ignore allocation that lead to exceptional return (oxcaml#1157) 1723728 flambda-backend: Re-enable parallel build of the runtime (oxcaml#1287) 26ea7f3 flambda-backend: Fix closure marshalling when not in NNP mode (oxcaml#1286) 9b91f2e flambda-backend: Reduce number of caml_apply functions taking/returning "I" and "V" (oxcaml#1272) 1686928 flambda-backend: Restore Cmm unboxing behaviour inside regions (oxcaml#1285) cf9be42 flambda-backend: Fix all the no-naked-pointers problems (oxcaml#1282) 8fe089e flambda-backend: Unrevert oxcaml#1131 and fix a Cmm unboxing bug (oxcaml#1284) c4143c3 flambda-backend: Revert "Make Selectgen treat region boundaries more precisely" (oxcaml#1283) 2078dce flambda-backend: Add some -dtimings output for the typechecker (oxcaml#1245) 273a7f9 flambda-backend: Make Selectgen treat region boundaries more precisely (oxcaml#1131) 47610e6 flambda-backend: Bump magic numbers for 4.14.1-6 (oxcaml#1274) fd53d38 flambda-backend: Generate *.cms files for merlin (oxcaml#1232) 853f95f flambda-backend: Add tail_mod_const to builtin_attrs (oxcaml#1265) f9ef051 flambda-backend: Fix issue caused by effects of gadt expansion in mode cross check (oxcaml#1263) e9ffcf8 flambda-backend: Fix dependencies for regenerating Flambda2 parser, tests (oxcaml#1255) 6f1cd1f flambda-backend: Restore a lost location, needed for merlin (oxcaml#1242) 009332b flambda-backend: Fix merge from ocaml-jst git-subtree-dir: ocaml git-subtree-split: a7d005a

gretay-js requested a review from xclerc as a code owner April 11, 2023 18:52

gretay-js requested a review from ccasin April 11, 2023 18:52

gretay-js force-pushed the alloc-check-warning-unused branch from 932de70 to 3f8f14e Compare April 12, 2023 08:53

gretay-js requested a review from mshinwell as a code owner April 12, 2023 08:53

gretay-js added 5 commits April 12, 2023 16:32

Add warning 199 for unchecked property attributes.

431f898

Builtin_attributes.warn_unused reports unchecked properties

27ff00f

Support for registering and marking property attributes.

Register property attributes and mark as checked

7cbef53

Add test

7a5b6b3

Warn about unchecked properties only in native and -zero-alloc-check

67f8160

The condition should be more general and check for the flag that correponds to the property attribute but for now we only have one.

gretay-js force-pushed the alloc-check-warning-unused branch from 3f8f14e to 67f8160 Compare April 12, 2023 15:36

ccasin approved these changes Apr 12, 2023

View reviewed changes

xclerc approved these changes Apr 13, 2023

View reviewed changes

gretay-js merged commit ba11c82 into oxcaml:main Apr 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zero_alloc new warning for unchecked functions#1302

zero_alloc new warning for unchecked functions#1302
gretay-js merged 5 commits intooxcaml:mainfrom
gretay-js:alloc-check-warning-unused

gretay-js commented Apr 11, 2023

Uh oh!

ccasin commented Apr 11, 2023

Uh oh!

lpw25 commented Apr 12, 2023

Uh oh!

gretay-js commented Apr 12, 2023

Uh oh!

gretay-js commented Apr 12, 2023

Uh oh!

ccasin left a comment

Uh oh!

xclerc left a comment •

edited

Loading

Uh oh!

gretay-js commented Apr 13, 2023

Uh oh!

gretay-js commented Apr 13, 2023 •

edited

Loading

Uh oh!

xclerc commented Apr 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

gretay-js commented Apr 11, 2023

Uh oh!

ccasin commented Apr 11, 2023

Uh oh!

lpw25 commented Apr 12, 2023

Uh oh!

gretay-js commented Apr 12, 2023

Uh oh!

gretay-js commented Apr 12, 2023

Uh oh!

ccasin left a comment

Choose a reason for hiding this comment

Uh oh!

xclerc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gretay-js commented Apr 13, 2023

Uh oh!

gretay-js commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xclerc commented Apr 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xclerc left a comment •

edited

Loading

gretay-js commented Apr 13, 2023 •

edited

Loading