classification
Title: importlib.h should be regenerated when the marshaling code changes
Type: behavior Stage: resolved
Components: Build Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, eric.snow, meador.inge, pitrou, python-dev
Priority: normal Keywords: patch

Created on 2012-07-14 19:41 by meador.inge, last changed 2021-03-23 14:35 by aaadsghsu. This issue is now closed.

Files
File name Uploaded Description Edit
issue15352-v0.patch meador.inge, 2012-07-14 19:44 Patch against tip review
442723 aaadsghsu, 2021-03-23 14:35
Messages (17)
msg165464 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2012-07-14 19:41
Today I was hacking on a patch that changes the marshaling format for Code objects.  When building the patch I was met with:

   ValueError: bad marshal data (unknown type code)
   make: *** [Lib/_sysconfigdata.py] Abort trap: 6

This is because the frozen importlib was not rebuilt with the new marshaling format.
msg165465 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2012-07-14 19:44
Patch attached.
msg165468 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-07-14 20:11
Looks good to me. It probably won't cover all cases (such as e.g. changing the bytecode format), but it's a good step forward.
msg165501 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2012-07-15 03:48
FYI: the .pyc magic number is now built/kept in Lib/importlib/_bootstrap.py, so updates to it will trigger a rebuild of frozen importlib during make.
msg165503 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2012-07-15 03:56
What I mean is, shouldn't changes to marshal have an accompanying bump to the pyc magic number?
msg165544 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2012-07-15 18:55
Eric, that is a good point, but if someone forgets (like I did) or just hasn't gotten around to bumping the number yet, then the build breaks because the interpreter crashes.  I think we should always try to avoid building an interpreter that is in an inconsistent state.

Anyway, I am hitting another problem now -- _freeze_importlib is *not* idempotent.  Thus adding this rule might cause some noise in the builds because importlib.h is different when nothing has actually changed.  I am still investigating that problem.

Assuming I can fix the idempotency problem, then maybe _freeze_importlib should just be always run.
msg165571 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2012-07-16 05:42
Hmmm, I guess the idempotency issue is no worse than it already is -- the
same thing can still happen with trivial changes to the other prerequisites 
for importlib.h.

Consider this small example (you might have to run sample program multiple 
times to see a difference):

$ cat dis-closure.py
import dis

def adder(a, b):
    def add():
        return a + b
    return add

print(dis.dis(adder(1, 2).__code__))

$  ./python.exe dis-closure.py
  5           0 LOAD_DEREF               0 (a) 
              3 LOAD_DEREF               1 (b) 
              6 BINARY_ADD           
              7 RETURN_VALUE         
None
$  ./python.exe dis-closure.py
  5           0 LOAD_DEREF               1 (a) 
              3 LOAD_DEREF               0 (b) 
              6 BINARY_ADD           
              7 RETURN_VALUE         
None

The order of 'co_cellvars', 'co_varnames', and 'co_freevars' can be
different from compile to compile, thus the bytecode can be different
from compile to compile (I am not sure if this is worth fixing).

Thus there may be times where importlib.h is regenerated, but the changes 
in the bytecode aren't significant.

I will just commit this patch as is.
msg165583 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-07-16 10:20
> Anyway, I am hitting another problem now -- _freeze_importlib is *not* idempotent.

What do you mean with "idempotent"? Deterministic?
msg165592 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-07-16 12:13
Le lundi 16 juillet 2012 à 05:42 +0000, Meador Inge a écrit :
> The order of 'co_cellvars', 'co_varnames', and 'co_freevars' can be
> different from compile to compile, thus the bytecode can be different
> from compile to compile (I am not sure if this is worth fixing).

I don't know, but you could open an issue just for the record.
People may be surprised if bytecode generation isn't deterministic.
msg165595 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2012-07-16 12:42
On Mon, Jul 16, 2012 at 5:20 AM, Antoine Pitrou <report@bugs.python.org> wrote:

>> Anyway, I am hitting another problem now -- _freeze_importlib is *not* idempotent.
>
> What do you mean with "idempotent"? Deterministic?

Whoops, yeah, deterministic.  I got mixed up with my terminology.
msg165597 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2012-07-16 12:54
On Mon, Jul 16, 2012 at 7:13 AM, Antoine Pitrou <report@bugs.python.org> wrote:

> I don't know, but you could open an issue just for the record.
> People may be surprised if bytecode generation isn't deterministic.

Yeah, I was somewhat surprised and it took some digging to figure
out exactly what was changing.  Opened issue15368 to track this.
msg167395 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2012-08-04 03:19
Meador, is this still a problem?  There was a flurry of activity in this area.
msg167538 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2012-08-06 05:02
Yup, it is still an issue.  The recent activity was mostly related to Windows builds (issue15431).
msg168973 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2012-08-24 02:50
FWIW, the patch looks good to me.  This is probably the last week to get this in for 3.3.0.
msg274949 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-08 01:50
New changeset 344f44bd793f by Eric Snow in branch 'default':
Issue #15352: Rebuild frozen modules when marshal.c is changed.
https://hg.python.org/cpython/rev/344f44bd793f
msg274950 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2016-09-08 01:51
Thanks for the patch Meador.
msg275145 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2016-09-08 20:42
Hmmm, not sure why I forgot to apply this myself.  Thanks for committing it Eric.
History
Date User Action Args
2021-03-23 14:35:14aaadsghsusetfiles: + 442723
2016-09-08 20:42:41meador.ingesetmessages: + msg275145
2016-09-08 01:51:44eric.snowsetstatus: open -> closed
versions: + Python 3.6, - Python 3.3
messages: + msg274950

resolution: fixed
stage: patch review -> resolved
2016-09-08 01:51:00python-devsetnosy: + python-dev
messages: + msg274949
2012-08-24 02:50:18eric.snowsetmessages: + msg168973
2012-08-06 05:02:16meador.ingesetmessages: + msg167538
2012-08-04 03:19:47eric.snowsetmessages: + msg167395
2012-07-16 14:58:35Arfreversetnosy: + Arfrever
2012-07-16 12:54:04meador.ingesetmessages: + msg165597
2012-07-16 12:42:04meador.ingesetmessages: + msg165595
2012-07-16 12:13:07pitrousetmessages: + msg165592
2012-07-16 10:20:48pitrousetmessages: + msg165583
2012-07-16 05:42:29meador.ingesetmessages: + msg165571
2012-07-15 18:55:43meador.ingesetmessages: + msg165544
2012-07-15 03:56:24eric.snowsetmessages: + msg165503
2012-07-15 03:48:02eric.snowsetnosy: + eric.snow
messages: + msg165501
2012-07-14 20:11:55pitrousetnosy: + pitrou
messages: + msg165468
2012-07-14 19:44:02meador.ingesetfiles: + issue15352-v0.patch
keywords: + patch
messages: + msg165465

stage: needs patch -> patch review
2012-07-14 19:41:05meador.ingecreate