Ticket #1382 (closed RFC: fixed)

Opened 5 years ago

Last modified 5 years ago

auto::alignptrs: Eliminate this config step

Reported by: jkeenan Owned by: jkeenan
Priority: normal Milestone:
Component: configure Version: trunk
Severity: medium Keywords:
Cc: plobsing Language:
Patch status: applied Platform:

Description

In TT #498, Andy Dougherty noted that the result of configuration step auto::alignptrs is used in only two PARROT_ASSERT statements in src/pmc/unmanagedstruct.pmc:

 127 static char * 
 128 char_offset_key(PARROT_INTERP, PMC *pmc, PMC *key, int *type) 
 129 {
 ...
 186             if (*type == enum_type_struct_ptr) {
 187                 /* that is either a pointer */
 188                 PARROT_ASSERT((PTR2INTVAL(p) & (PARROT_PTR_ALIGNMENT - 1)) ==      0);
 189                 VTABLE_set_pointer(interp, init, *(void**)p);
 190             }
 ...
 196         else if (init->vtable->base_type == enum_class_ManagedStruct
 197              && *type                    == enum_type_struct_ptr) {
 198             /* a nested struct pointer belonging to us
 199              * p is the location of the struct pointer in the
 200              * outer struct, the inner is at PMC_data(init) */
 201 
 202             PARROT_ASSERT((PTR2INTVAL(p) & (PARROT_PTR_ALIGNMENT - 1)) == 0);
 203             *(void **)p = VTABLE_get_pointer(interp, init);
 204         }
 205 
 206         return char_offset_key(interp, init, next, type);
 207     }

So tonight I created the noalignptrs branch to see what would happen if we did indeed eliminate this step. So far, it has built and passed make test.

Attachments

noalignptrs.diff.gz Download (3.4 KB) - added by jkeenan 5 years ago.
diff between noalignptrs branch and trunk

Change History

in reply to: ↑ description ; follow-up: ↓ 2   Changed 5 years ago by jkeenan

Replying to jkeenan:

The only failure I got during make fulltest was the test of examples/pir/quine_ord.pir in t/examples/pir.t. The output of this test is massive, so I'm not going to post it. Investigating.

kid51

in reply to: ↑ 1   Changed 5 years ago by jkeenan

  • status changed from new to assigned
  • owner set to jkeenan

Replying to jkeenan:

Replying to jkeenan: The only failure I got during make fulltest was the test of examples/pir/quine_ord.pir in t/examples/pir.t.

Ah, not my fault! This is failing in trunk, observed in r43153.

Changed 5 years ago by jkeenan

diff between noalignptrs branch and trunk

  Changed 5 years ago by jkeenan

I've attached a diff between branch and trunk which should be considered as a first draft of a patch implementing the removal of auto::alignptrs. So far, all I've done is to comment out the two PARROT_ASSERT lines.

Index: src/pmc/unmanagedstruct.pmc
===================================================================
--- src/pmc/unmanagedstruct.pmc (revision 43151)
+++ src/pmc/unmanagedstruct.pmc (revision 43153)
@@ -185,7 +185,9 @@
             /* now point ptr to the real data */
             if (*type == enum_type_struct_ptr) {
                 /* that is either a pointer */
+                /*
                 PARROT_ASSERT((PTR2INTVAL(p) & (PARROT_PTR_ALIGNMENT - 1)) == 0);
+                */
                 VTABLE_set_pointer(interp, init, *(void**)p);
             }
 
@@ -199,7 +201,9 @@
              * p is the location of the struct pointer in the
              * outer struct, the inner is at PMC_data(init) */
 
+            /*
             PARROT_ASSERT((PTR2INTVAL(p) & (PARROT_PTR_ALIGNMENT - 1)) == 0);
+            */
             *(void **)p = VTABLE_get_pointer(interp, init);
         }

I suspect that deleting those two lines, including their reference to PTR2INTVAL is probably not the best way to proceed, even if it works. Should there be some other PARROT_ASSERT there?

Suggestions?

Thank you very much.
kid51

follow-up: ↓ 5   Changed 5 years ago by jkeenan

  • cc plobsing added

Paraphrasing some comments from plobsing on #parrot:

The assertions at lines 188 and 202 of src/pmc/unmanagedstruct.pmc (trunk) check that the pointers are aligned to a 2**n char boundary (required on some obscure platform I'm sure).

(But it appears we have yet to encounter such a platform. -- kid51)

On the one hand, parrot doesn't check this elsewhere; on the other, UnManagedStruct is the closest users come to directly managing pointers, so extra checks might be good. We could say: "If you run a platform that breaks if you access pointers that aren't aligned and you break it using UnManagedStruct, that's your fault." In a way, UnManagedStruct is part of the escape hatch of the VM abstraction.

Seeking more comments from knowledgeable sources.

kid51

in reply to: ↑ 4 ; follow-up: ↓ 7   Changed 5 years ago by doughera

Replying to jkeenan:

Paraphrasing some comments from plobsing on #parrot: The assertions at lines 188 and 202 of src/pmc/unmanagedstruct.pmc (trunk) check that the pointers are aligned to a 2**n char boundary (required on some obscure platform I'm sure). (But it appears we have yet to encounter such a platform. -- kid51)

Actually it's a reasonably common requirement on RISC processors. SPARC is one well-known example, but there are many others. It's more that x86 doesn't require any such alignment constraints, so many programmers are unfamiliar with them.

I suspect that these checks are a holdover from when Parrot tried to manage alignment manually in many cases. It does less of that now. If pointers are allocated and used appropriately, then the compiler will guarantee that they are suitably aligned for their stated purpose, and no assert() checks are required.

I'd say "Just delete the lines."

  Changed 5 years ago by doughera

  • cc doughera removed

in reply to: ↑ 5   Changed 5 years ago by jkeenan

Replying to doughera:

I'd say "Just delete the lines."

Thanks, Andy. I will do so and merge the noalignptrs branch into trunk after tomorrow's release.

kid51

  Changed 5 years ago by jkeenan

  • version changed from 1.9.0 to trunk
  • patch set to applied

Merged into trunk at r43490. I will close ticket no later than this weekend.

Thank you very much.

kid51

  Changed 5 years ago by jkeenan

  • status changed from assigned to closed
  • resolution set to fixed

No complaints; closing ticket.

Thank you very much.

kid51

Note: See TracTickets for help on using tickets.