1472121 - Remove unused patches from build/build-clang/

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Marco Castelluccio [:marco]]

There are several patches that we no longer use as they were upstreamed, e.g. hide-gcda-profiling-symbols.patch.

Comment 1

[Mike Hommey [:glandium]]

It's still applied in clang-win64-st-an.json

Comment 2

[Marco Castelluccio [:marco]]

(In reply to Mike Hommey [:glandium] from comment #1)
> It's still applied in clang-win64-st-an.json

It was only needed for the coverage build, I don't know why we're applying it there but I think we can safely stop.

Comment 3

[Devika Sugathan]

Hi,
I'm an absolute beginner and would like to take up this bug.

Comment 4

[Marco Castelluccio [:marco]]

Hello Devika! Feel free to work on this! Basically, you'll need to find the patches in https://hg.mozilla.org/mozilla-central/file/tip/build/build-clang which are not listed in any of the JSON files, and remove those patch files.
Also, you can remove hide-gcda-profiling-symbols.patch from clang-win64-st-an.json, and remove the clang-win64-st-an.json file.

Comment 5

[Devika Sugathan]

I have made the changes.

Comment 6

[Marco Castelluccio [:marco]]

(In reply to Devika Sugathan from comment #5)
> I have made the changes.

Great, thanks! You can upload the patch here and ask for review.

Comment 7

[Devika Sugathan]

Attachment: Removed.patch

Comment 8

[Marco Castelluccio [:marco]]

Comment on attachment 8995963 [details] [diff] [review]
Removed.patch

Sorry Devika, there was a spelling mistake in my comment:

(In reply to Marco Castelluccio [:marco] from comment #4)
> Also, you can remove hide-gcda-profiling-symbols.patch from
> clang-win64-st-an.json, and remove the clang-win64-st-an.json file.

should have been:

> Also, you can remove hide-gcda-profiling-symbols.patch from
> clang-win64-st-an.json, and remove the **hide-gcda-profiling-symbols.patch** file.

Comment 9

[Marco Castelluccio [:marco]]

Note, you should ask for review by setting review as ? and putting the email of a developer. review+ is assigned when your patch has been reviewed positively.

Comment 10

[Devika Sugathan]

Attachment: removed.patch

Comment 11

[Marco Castelluccio [:marco]]

Comment on attachment 8995991 [details] [diff] [review]
removed.patch

The patch doesn't seem to apply cleanly, maybe you built this on top of the other?
You can use `hg histedit` to merge them together.

Comment 12

[Devika Sugathan]

Attachment: Changed.patch

Comment 13

[Marco Castelluccio [:marco]]

Comment on attachment 8996004 [details] [diff] [review]
Changed.patch

Review of attachment 8996004 [details] [diff] [review]:
-----------------------------------------------------------------

Same problem as before! You can double check the patch before uploading it :)

Comment 14

[Devika Sugathan]

Attachment: New.patch

Comment 15

[Marco Castelluccio [:marco]]

Comment on attachment 8996043 [details] [diff] [review]
New.patch

Review of attachment 8996043 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!
Are you sure you got all patches that were not referenced?

Comment 16

[Devika Sugathan]

Yes I have.

Comment 17

[Devika Sugathan]

I have removed patch file in build-clang which was not referenced in any of the JSON files. If I'm not wrong then there are only two files to remove.

Comment 18

[Marco Castelluccio [:marco]]

(In reply to Marco Castelluccio [:marco] from comment #15)
> Comment on attachment 8996043 [details] [diff] [review]
> New.patch
> 
> Review of attachment 8996043 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, thanks!
> Are you sure you got all patches that were not referenced?

Tests are green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3202eda70cbca5b2afa574ec9e78246f1d5c086c.

(In reply to Devika Sugathan from comment #17)
> I have removed patch file in build-clang which was not referenced in any of
> the JSON files. If I'm not wrong then there are only two files to remove.

Sounds good, thanks! Only one thing missing:

The commit message should be updated, it should be in this format:
Bug BUG_NUMBER - DESCRIPTION OF THE CHANGES. r=REVIEWER

So in this case:
Bug 1472121 - Remove unused patches from build/build-clang/. r=marco

Comment 19

[Devika Sugathan]

Comment on attachment 8996043 [details] [diff] [review]
New.patch

># HG changeset patch
># User Devika Sugathan <[e-mail]>
># Date 1532977536 -19800
>#      Tue Jul 31 00:35:36 2018 +0530
># Node ID eb6cb4b07211a7ef4ddc6419c1e34dcecdc454c1
># Parent  07534a3e7da4bc68e2494473badb787d0481da1d
>Bug 1472121 - Remove unused patches from build/build-clang/. r=marco
>
>diff --git a/build/build-clang/clang-win64-st-an.json b/build/build-clang/clang-win64-st-an.json
>--- a/build/build-clang/clang-win64-st-an.json
>+++ b/build/build-clang/clang-win64-st-an.json
>@@ -17,7 +17,6 @@
>       "r318309.patch",
>       "r320462.patch",
>       "loosen-msvc-detection.patch",
>-      "hide-gcda-profiling-symbols.patch",
>       "fflush-before-unlocking.patch"
>     ]
> }
>diff --git a/build/build-clang/hide-gcda-profiling-symbols.patch b/build/build-clang/hide-gcda-profiling-symbols.patch
>deleted file mode 100644
>--- a/build/build-clang/hide-gcda-profiling-symbols.patch
>+++ /dev/null
>@@ -1,108 +0,0 @@
>-diff --git a/compiler-rt/lib/profile/GCDAProfiling.c b/compiler-rt/lib/profile/GCDAProfiling.c
>-index 138af6ec4..f0c05075a 100644
>---- a/compiler-rt/lib/profile/GCDAProfiling.c
>-+++ b/compiler-rt/lib/profile/GCDAProfiling.c
>-@@ -231,6 +231,7 @@ static void unmap_file() {
>-  * profiling enabled will emit to a different file. Only one file may be
>-  * started at a time.
>-  */
>-+COMPILER_RT_VISIBILITY
>- void llvm_gcda_start_file(const char *orig_filename, const char version[4],
>-                           uint32_t checksum) {
>-   const char *mode = "r+b";
>-@@ -298,6 +299,7 @@ void llvm_gcda_start_file(const char *orig_filename, const char version[4],
>- /* Given an array of pointers to counters (counters), increment the n-th one,
>-  * where we're also given a pointer to n (predecessor).
>-  */
>-+COMPILER_RT_VISIBILITY
>- void llvm_gcda_increment_indirect_counter(uint32_t *predecessor,
>-                                           uint64_t **counters) {
>-   uint64_t *counter;
>-@@ -320,6 +322,7 @@ void llvm_gcda_increment_indirect_counter(uint32_t *predecessor,
>- #endif
>- }
>- 
>-+COMPILER_RT_VISIBILITY
>- void llvm_gcda_emit_function(uint32_t ident, const char *function_name,
>-                              uint32_t func_checksum, uint8_t use_extra_checksum,
>-                              uint32_t cfg_checksum) {
>-@@ -346,6 +349,7 @@ void llvm_gcda_emit_function(uint32_t ident, const char *function_name,
>-     write_string(function_name);
>- }
>- 
>-+COMPILER_RT_VISIBILITY
>- void llvm_gcda_emit_arcs(uint32_t num_counters, uint64_t *counters) {
>-   uint32_t i;
>-   uint64_t *old_ctrs = NULL;
>-@@ -397,6 +401,7 @@ void llvm_gcda_emit_arcs(uint32_t num_counters, uint64_t *counters) {
>- #endif
>- }
>- 
>-+COMPILER_RT_VISIBILITY
>- void llvm_gcda_summary_info() {
>-   const uint32_t obj_summary_len = 9; /* Length for gcov compatibility. */
>-   uint32_t i;
>-@@ -450,6 +455,7 @@ void llvm_gcda_summary_info() {
>- #endif
>- }
>- 
>-+COMPILER_RT_VISIBILITY
>- void llvm_gcda_end_file() {
>-   /* Write out EOF record. */
>-   if (output_file) {
>-@@ -474,6 +480,7 @@ void llvm_gcda_end_file() {
>- #endif
>- }
>- 
>-+COMPILER_RT_VISIBILITY
>- void llvm_register_writeout_function(writeout_fn fn) {
>-   struct writeout_fn_node *new_node = malloc(sizeof(struct writeout_fn_node));
>-   new_node->fn = fn;
>-@@ -487,6 +494,7 @@ void llvm_register_writeout_function(writeout_fn fn) {
>-   }
>- }
>- 
>-+COMPILER_RT_VISIBILITY
>- void llvm_writeout_files(void) {
>-   struct writeout_fn_node *curr = writeout_fn_head;
>- 
>-@@ -496,6 +504,7 @@ void llvm_writeout_files(void) {
>-   }
>- }
>- 
>-+COMPILER_RT_VISIBILITY
>- void llvm_delete_writeout_function_list(void) {
>-   while (writeout_fn_head) {
>-     struct writeout_fn_node *node = writeout_fn_head;
>-@@ -506,6 +515,7 @@ void llvm_delete_writeout_function_list(void) {
>-   writeout_fn_head = writeout_fn_tail = NULL;
>- }
>- 
>-+COMPILER_RT_VISIBILITY
>- void llvm_register_flush_function(flush_fn fn) {
>-   struct flush_fn_node *new_node = malloc(sizeof(struct flush_fn_node));
>-   new_node->fn = fn;
>-@@ -519,6 +529,7 @@ void llvm_register_flush_function(flush_fn fn) {
>-   }
>- }
>- 
>-+COMPILER_RT_VISIBILITY
>- void __gcov_flush() {
>-   struct flush_fn_node *curr = flush_fn_head;
>- 
>-@@ -528,6 +539,7 @@ void __gcov_flush() {
>-   }
>- }
>- 
>-+COMPILER_RT_VISIBILITY
>- void llvm_delete_flush_function_list(void) {
>-   while (flush_fn_head) {
>-     struct flush_fn_node *node = flush_fn_head;
>-@@ -538,6 +550,7 @@ void llvm_delete_flush_function_list(void) {
>-   flush_fn_head = flush_fn_tail = NULL;
>- }
>- 
>-+COMPILER_RT_VISIBILITY
>- void llvm_gcov_init(writeout_fn wfn, flush_fn ffn) {
>-   static int atexit_ran = 0;
>- 
>diff --git a/build/build-clang/return-empty-string-non-mangled.patch b/build/build-clang/return-empty-string-non-mangled.patch
>deleted file mode 100644
>--- a/build/build-clang/return-empty-string-non-mangled.patch
>+++ /dev/null
>@@ -1,19 +0,0 @@
>-Author: Michael Wu <[e-mail]>
>-Date:   Thu Sep 24 11:36:08 2015 -0400
>-
>-    Return an empty string when a symbol isn't mangled
>-
>-diff --git a/clang/tools/libclang/CIndex.cpp b/clang/tools/libclang/CIndex.cpp
>---- a/clang/tools/libclang/CIndex.cpp
>-+++ b/clang/tools/libclang/CIndex.cpp
>-@@ -3990,6 +3990,10 @@
>-   ASTContext &Ctx = ND->getASTContext();
>-   std::unique_ptr<MangleContext> MC(Ctx.createMangleContext());
>- 
>-+  // Don't mangle if we don't need to.
>-+  if (!MC->shouldMangleCXXName(ND))
>-+    return cxstring::createEmpty();
>-+
>-   std::string FrontendBuf;
>-   llvm::raw_string_ostream FrontendBufOS(FrontendBuf);
>-   if (MC->shouldMangleDeclName(ND)) {

Comment 20

[Devika Sugathan]

Should I attach this as a new patch with updated details?

Comment 21

[Marco Castelluccio [:marco]]

(In reply to Devika Sugathan from comment #20)
> Should I attach this as a new patch with updated details?

Yes, please attach it as a new patch, marking the old ones as "obsolete".

Comment 22

[Devika Sugathan]

Attachment: Updated.patch

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71835e9faee8
Remove unused patches from build/build-clang/. r=marco

Comment 24

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/71835e9faee8

Comment 25

[Devika Sugathan]

Can you suggest me another beginner bug to work on?

Comment 26

[Marco Castelluccio [:marco]]

Congratulations for your contribution!

(In reply to Devika Sugathan from comment #25)
> Can you suggest me another beginner bug to work on?

You can find many on bugsahoy (https://www.joshmatthews.net/bugsahoy/), where you can also filter for your interests and language knowledge.