[PATCH] D60379: Make precompiled headers reproducible

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru abandoned this revision.
sylvestre.ledru added a comment.

yeah
thanks


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60379/new/

https://reviews.llvm.org/D60379



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60379: Make precompiled headers reproducible

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

Can this be closed now due to commit r358674?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60379/new/

https://reviews.llvm.org/D60379



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60379: Make precompiled headers reproducible

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

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?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60379/new/

https://reviews.llvm.org/D60379



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60379: Make precompiled headers reproducible

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: lib/Serialization/ASTWriter.cpp:4283
+  // Sort to allow reproducible .pch files - https://bugs.debian.org/877359
+  std::map> sortedOpenCLTypeExtMap;
   for (const auto &I : SemaRef.OpenCLTypeExtMap) {

riccibruno wrote:
> lebedev.ri wrote:
> > Anastasia wrote:
> > > lebedev.ri wrote:
> > > > Would it be better to just change the actual type of 
> > > > `SemaRef.OpenCLTypeExtMap`?
> > > > https://github.com/llvm-mirror/clang/blob/18917301298ad6df9e989983ed1e22cb0f9dff29/include/clang/Sema/Sema.h#L8710-L8712
> > > 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.
> > Just `std::map<>`?
> 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.
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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60379/new/

https://reviews.llvm.org/D60379



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60379: Make precompiled headers reproducible

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D60379#1458799 , @thakis wrote:

> In D60379#1458668 , @riccibruno 
> wrote:
>
> > 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).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60379/new/

https://reviews.llvm.org/D60379



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60379: Make precompiled headers reproducible

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

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


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60379/new/

https://reviews.llvm.org/D60379



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60379: Make precompiled headers reproducible

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

In D60379#1458668 , @riccibruno wrote:

> 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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60379/new/

https://reviews.llvm.org/D60379



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60379: Make precompiled headers reproducible

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60379/new/

https://reviews.llvm.org/D60379



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60379: Make precompiled headers reproducible

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: lib/Serialization/ASTWriter.cpp:4283
+  // Sort to allow reproducible .pch files - https://bugs.debian.org/877359
+  std::map> sortedOpenCLTypeExtMap;
   for (const auto &I : SemaRef.OpenCLTypeExtMap) {

lebedev.ri wrote:
> Anastasia wrote:
> > lebedev.ri wrote:
> > > Would it be better to just change the actual type of 
> > > `SemaRef.OpenCLTypeExtMap`?
> > > https://github.com/llvm-mirror/clang/blob/18917301298ad6df9e989983ed1e22cb0f9dff29/include/clang/Sema/Sema.h#L8710-L8712
> > 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.
> Just `std::map<>`?
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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60379/new/

https://reviews.llvm.org/D60379



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60379: Make precompiled headers reproducible

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/Serialization/ASTWriter.cpp:4283
+  // Sort to allow reproducible .pch files - https://bugs.debian.org/877359
+  std::map> sortedOpenCLTypeExtMap;
   for (const auto &I : SemaRef.OpenCLTypeExtMap) {

Anastasia wrote:
> lebedev.ri wrote:
> > Would it be better to just change the actual type of 
> > `SemaRef.OpenCLTypeExtMap`?
> > https://github.com/llvm-mirror/clang/blob/18917301298ad6df9e989983ed1e22cb0f9dff29/include/clang/Sema/Sema.h#L8710-L8712
> 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.
Just `std::map<>`?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60379/new/

https://reviews.llvm.org/D60379



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60379: Make precompiled headers reproducible

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: lib/Serialization/ASTWriter.cpp:4283
+  // Sort to allow reproducible .pch files - https://bugs.debian.org/877359
+  std::map> sortedOpenCLTypeExtMap;
   for (const auto &I : SemaRef.OpenCLTypeExtMap) {

lebedev.ri wrote:
> Would it be better to just change the actual type of 
> `SemaRef.OpenCLTypeExtMap`?
> https://github.com/llvm-mirror/clang/blob/18917301298ad6df9e989983ed1e22cb0f9dff29/include/clang/Sema/Sema.h#L8710-L8712
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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60379/new/

https://reviews.llvm.org/D60379



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60379: Make precompiled headers reproducible

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

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


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60379/new/

https://reviews.llvm.org/D60379



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60379: Make precompiled headers reproducible

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Needs a test.




Comment at: lib/Serialization/ASTWriter.cpp:4283
+  // Sort to allow reproducible .pch files - https://bugs.debian.org/877359
+  std::map> sortedOpenCLTypeExtMap;
   for (const auto &I : SemaRef.OpenCLTypeExtMap) {

Would it be better to just change the actual type of `SemaRef.OpenCLTypeExtMap`?
https://github.com/llvm-mirror/clang/blob/18917301298ad6df9e989983ed1e22cb0f9dff29/include/clang/Sema/Sema.h#L8710-L8712


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60379/new/

https://reviews.llvm.org/D60379



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60379: Make precompiled headers reproducible

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru created this revision.
sylvestre.ledru added reviewers: yaxunl, Anastasia.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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

Patch by Rebecca N. Palmer


Repository:
  rC Clang

https://reviews.llvm.org/D60379

Files:
  lib/Serialization/ASTWriter.cpp


Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -4279,9 +4279,13 @@
 return;
 
   RecordData Record;
+  // Sort to allow reproducible .pch files - https://bugs.debian.org/877359
+  std::map> sortedOpenCLTypeExtMap;
   for (const auto &I : SemaRef.OpenCLTypeExtMap) {
-Record.push_back(
-static_cast(getTypeID(I.first->getCanonicalTypeInternal(;
+
sortedOpenCLTypeExtMap[getTypeID(I.first->getCanonicalTypeInternal())]=I.second;
+  }
+  for (const auto &I : sortedOpenCLTypeExtMap) {
+Record.push_back(static_cast(I.first));
 Record.push_back(I.second.size());
 for (auto Ext : I.second)
   AddString(Ext, Record);
@@ -4294,8 +4298,12 @@
 return;
 
   RecordData Record;
+  std::map> sortedOpenCLDeclExtMap;
   for (const auto &I : SemaRef.OpenCLDeclExtMap) {
-Record.push_back(getDeclID(I.first));
+sortedOpenCLDeclExtMap[getDeclID(I.first)]=I.second;
+  }
+  for (const auto &I : sortedOpenCLDeclExtMap) {
+Record.push_back(I.first);
 Record.push_back(static_cast(I.second.size()));
 for (auto Ext : I.second)
   AddString(Ext, Record);


Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -4279,9 +4279,13 @@
 return;
 
   RecordData Record;
+  // Sort to allow reproducible .pch files - https://bugs.debian.org/877359
+  std::map> sortedOpenCLTypeExtMap;
   for (const auto &I : SemaRef.OpenCLTypeExtMap) {
-Record.push_back(
-static_cast(getTypeID(I.first->getCanonicalTypeInternal(;
+sortedOpenCLTypeExtMap[getTypeID(I.first->getCanonicalTypeInternal())]=I.second;
+  }
+  for (const auto &I : sortedOpenCLTypeExtMap) {
+Record.push_back(static_cast(I.first));
 Record.push_back(I.second.size());
 for (auto Ext : I.second)
   AddString(Ext, Record);
@@ -4294,8 +4298,12 @@
 return;
 
   RecordData Record;
+  std::map> sortedOpenCLDeclExtMap;
   for (const auto &I : SemaRef.OpenCLDeclExtMap) {
-Record.push_back(getDeclID(I.first));
+sortedOpenCLDeclExtMap[getDeclID(I.first)]=I.second;
+  }
+  for (const auto &I : sortedOpenCLDeclExtMap) {
+Record.push_back(I.first);
 Record.push_back(static_cast(I.second.size()));
 for (auto Ext : I.second)
   AddString(Ext, Record);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits