Page MenuHomePhabricator

Make precompiled headers reproducible
AbandonedPublic

Authored by sylvestre.ledru on Apr 7 2019, 3:07 PM.

Details

Reviewers
yaxunl
Anastasia
Summary

This patch have been applied to Debian packages for a few months without
any regression.

https://bugs.debian.org/877359

Patch by Rebecca N. Palmer

Diff Detail

Event Timeline

sylvestre.ledru created this revision.Apr 7 2019, 3:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2019, 3:07 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Needs a test.

lib/Serialization/ASTWriter.cpp
4283
sylvestre.ledru edited the summary of this revision. (Show Details)Apr 8 2019, 1:33 AM

I won't have time to write the test for that (and would not know where to start). I am just the messenger :)

Anastasia added inline comments.Apr 8 2019, 10:25 AM
lib/Serialization/ASTWriter.cpp
4283

What data structure would you suggest to use instead? I think sorting during serialization or de-serialization of AST isn't too bad because it's not a common use case.

lebedev.ri added inline comments.Apr 8 2019, 10:32 AM
lib/Serialization/ASTWriter.cpp
4283

Just std::map<>?

riccibruno added inline comments.
lib/Serialization/ASTWriter.cpp
4283

Why is this a problem ? I thought that pch files where not supposed to be distributed. If we come to the conclusion that pch files must be reproducible, then I believe that this is by far not the only example.

After looking at the bug report (https://bugs.debian.org/877359), I would like to point out that there is currently *absolutely* no stability guarantee for the serialization format (ie, you need the exact same revision). In regard of this I am wondering how sane it is to ship pch files.

After looking at the bug report (https://bugs.debian.org/877359), I would like to point out that there is currently *absolutely* no stability guarantee for the serialization format (ie, you need the exact same revision). In regard of this I am wondering how sane it is to ship pch files.

I agree we shouldn't make guarantees about pch file format stability and that distributing pch files isn't a good idea. Having said that, making clang write the same output when given the same input is still a Good Thing as it enables caching these outputs e.g. with tools like distcc.

To be clear, this isn't about format stability and distributing but making sure that two same runs of clang produces the same output.

After looking at the bug report (https://bugs.debian.org/877359), I would like to point out that there is currently *absolutely* no stability guarantee for the serialization format (ie, you need the exact same revision). In regard of this I am wondering how sane it is to ship pch files.

I agree we shouldn't make guarantees about pch file format stability and that distributing pch files isn't a good idea. Having said that, making clang write the same output when given the same input is still a Good Thing as it enables caching these outputs e.g. with tools like distcc.

Good point.

I went through each of the structures serialized in ASTWriter.cpp, and unless I missed one, OpenCLDeclExtMap and OpenCLTypeExtMap are the only one which are serialized in a non-deterministic way (so my earlier inline comment was mistaken).

Anastasia added inline comments.Apr 9 2019, 4:57 AM
lib/Serialization/ASTWriter.cpp
4283

Ok, I would normally prefer to use DenseMap for the general case because since the number of elements in OpenCLTypeExtMap is very small we can take advantage of non-heap allocation. But at the same time probably a couple of elements won't make any difference for std::map either and we can keep the code base more maintainable. So I don't mind either way.

FYI, I switched to std::map in this review:
https://reviews.llvm.org/D60778

@sylvestre.ledru do you think it's possible to apply this patch and see if it work for your bug?

Btw I am still confused about the reproducibility - does clang produce the same PCH at least for the runs on the same revision?

Can this be closed now due to commit r358674?

sylvestre.ledru abandoned this revision.Jun 8 2019, 7:42 AM

yeah
thanks