1272498 - Compiling with GCC and Address Sanitizer is broken: "error: expected unqualified-id before string constant"

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Georg Koppen]

After bug 1252072 landed compiling with GCC and Address Sanitizer is broken:

/usr/bin/g++ -std=gnu++11 -o xptcinvoke_x86_64_unix.o -c -I/home/thomas/Arbeit/Tor/mozilla-central/obj-x86_64-pc-linux-gnu/dist/stl_wrappers -I/home/thomas/Arbeit/Tor/mozilla-central/obj-x86_64-pc-linux-gnu/dist/system_wrappers -include /home/thomas/Arbeit/Tor/mozilla-central/config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/home/thomas/Arbeit/Tor/mozilla-central/xpcom/reflect/xptcall/md/unix -I/home/thomas/Arbeit/Tor/mozilla-central/obj-x86_64-pc-linux-gnu/xpcom/reflect/xptcall/md/unix -I/home/thomas/Arbeit/Tor/mozilla-central/xpcom/reflect/xptcall -I/home/thomas/Arbeit/Tor/mozilla-central/xpcom/reflect/xptinfo -I/home/thomas/Arbeit/Tor/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include  -I/home/thomas/Arbeit/Tor/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nspr -I/home/thomas/Arbeit/Tor/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nss       -fPIC  -DMOZILLA_CLIENT -include /home/thomas/Arbeit/Tor/mozilla-central/obj-x86_64-pc-linux-gnu/mozilla-config.h -MD -MP -MF .deps/xptcinvoke_x86_64_unix.o.pp  -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wc++14-compat -Wno-invalid-offsetof -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -fsanitize=address -Dxmalloc=myxmalloc -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -pthread -pipe  -g -freorder-blocks -Os -fno-omit-frame-pointer   -Wshadow  /home/thomas/Arbeit/Tor/mozilla-central/xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp

In file included from /home/thomas/Arbeit/Tor/mozilla-central/xpcom/reflect/xptcall/xptcall.h:11:0,
                 from /home/thomas/Arbeit/Tor/mozilla-central/xpcom/reflect/xptcall/xptcprivate.h:11,
                 from /home/thomas/Arbeit/Tor/mozilla-central/xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:9:
/home/thomas/Arbeit/Tor/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nscore.h:185:28: error: expected unqualified-id before string constant
 #define NS_EXTERN_C extern "C"
                            ^
/home/thomas/Arbeit/Tor/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nscore.h:190:32: note: in expansion of macro ‘NS_EXTERN_C’
 #define EXPORT_XPCOM_API(type) NS_EXTERN_C NS_EXPORT type NS_FROZENCALL
                                ^
/home/thomas/Arbeit/Tor/mozilla-central/xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:122:1: note: in expansion of macro ‘EXPORT_XPCOM_API’
 EXPORT_XPCOM_API(nsresult)
 ^

Comment 1

[Trevor Saunders (:tbsaunde)]

presumably gcc doesn't like __attribute__ before extern "c".  Does it work to put the MOZ_ASAN_BLACKLIST after the XPCOM_API() ?

Comment 2

[Georg Koppen]

Yes, that fixes the build bustage, thanks. I can prepare a patch but am wondering whether there is a way to do a try build making sure clang is still happy with it. Or would it be enough to use a local build for that?

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to Georg Koppen from comment #2)
> Yes, that fixes the build bustage, thanks. I can prepare a patch but am
> wondering whether there is a way to do a try build making sure clang is
> still happy with it. Or would it be enough to use a local build for that?

I'm pretty confident clang is fine with that, but any build on try that uses clang should confirm that, presumably clang will complain if it doesn't like where you put the attribute.

Comment 4

[ISHIKAWA, Chiaki]

Hi, has the patch been produced and proposed?
I hit the same issue while trying to build ASAN-enabled C-C thunderbird.
And I found the same fix.
https://groups.google.com/forum/#!topic/mozilla.dev.apps.thunderbird/_KulJ0pH4MQ
It would be great if the fix can land in the M-C tree soon.

Regarding the try build, one of these days, doesn't try support enabling ASAN?
http://trychooser.pub.build.mozilla.org/

I see "linux-64 asan" option.

So will my put in a C-C TB tryserver job with the patch for this issue in M-C tree portion will be enough for the check? I assume that try server uses clang.

TIA


TIA

Comment 5

[ISHIKAWA, Chiaki]

I tried to run C-C TB tryserver build with linux64-asan, but it seems to get stuck?
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c903576da092519f2c58bc8a656a92928be85f1a

So I suspect that C-C tryserver may not support building asan version.

I am not sure if I can build mozilla firefox: not sure if I have the right source tree, but let me build ASAN version on try. Stay tuned.

I saw some jobs on try, that seems to trigger linux64-asan build, and not sure what is the difference between such submitters and the original reporter and myself, BTW.


TAI

Comment 6

[ISHIKAWA, Chiaki]

Attachment: Bug 1272498 - put __attribute__((id)__ AFTER extern "C" to avoid ASAN-build bustage with GCC

https://treeherder.mozilla.org/#/jobs?repo=try&revision=87ed181fae4e0ce9e73b1e96ec4159be944a2c83

Please see the above, the patch there is 
https://hg.mozilla.org/try/rev/6f6b016d8ece61228769355f768ec6475c2a7b9b

I am going to propose it for inclusion in M-C tree with minor commit message change.

TIA

Comment 7

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8764936 [details] [diff] [review]
Bug 1272498 - put __attribute__((id)__ AFTER extern "C" to avoid ASAN-build bustage with GCC

this is fine, but I think its high time we just rewrote the crazy part of this in assembly so I'm doingthat.

Comment 8

[Trevor Saunders (:tbsaunde)]

Attachment: rewrite NS_InvokeByIndex in assembly

This could probably use some cleanup, andI'm still hunting a bug in it somewhere, but I think its at a point where it can be looked at.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8766021 [details] [diff] [review]
rewrite NS_InvokeByIndex in assembly

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

Seems reasonable, some comments below.

::: xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.s
@@ +1,3 @@
> +.intel_syntax noprefix
> +.global NS_InvokeByIndex
> +NS_InvokeByIndex:

Please make sure this is a hidden symbol in the final libxul, and not a globally visible one.  You probably need to set visibility explicitly in this file.

You should also say that you're in the .text section, and specifying an alignment for this function would be good.  A comment with the prototype of this function would be most welcome for future readers.

@@ +6,5 @@
> +
> +  push r12
> +  push r13
> +  mov r12, rdi
> +  mov r13, rsi

Other blocks are commented; this block could use a comment or two.

@@ +20,5 @@
> +# setup space for the register slots, there are 5 integer ones and 8 floating
> +# point ones.
> +  sub rsp, 112
> +  mov rdi, rsp
> +  lea rsi, [rsp + 40]

This mov/lea combo probably warrants a separate comment.

@@ +42,5 @@
> +  movsd xmm3, [rsp + 64]
> +  movsd xmm4, [rsp + 72]
> +  movsd xmm5, [rsp + 80]
> +  movsd xmm6, [rsp + 88]
> +  movsd xmm7, [rsp + 96]

You only use 104 bytes of stack space for the registers, but set up 112?  Is 112 for stack alignment purposes, given the ABI?  Would be good to note that.

@@ +57,5 @@
> +  pop r12
> +
> +  mov rsp, rbp
> +  pop rbp
> +  ret

This file is going to need some no-executable-stack magic in it.

Comment 10

[Trevor Saunders (:tbsaunde)]

Attachment: rewrite NS_InvokeByIndex in assembly

Comment 11

[Trevor Saunders (:tbsaunde)]

Attachment: rewrite NS_InvokeByIndex in assembly

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8771012 [details] [diff] [review]
rewrite NS_InvokeByIndex in assembly

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

r=me with changes as noted below.  I assume this passes tests?

::: xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.s
@@ +1,1 @@
> +.intel_syntax noprefix

This file needs an MPL license header.

@@ +2,5 @@
> +
> +# nsresult NS_InvokeByIndex(nsISupports* this, uint32_t aVtableIndex,
> +#                           uint32_t argc, nsXPTCVariant* argv);
> +.text
> +.global NS_InvokeByIndex

This symbol is default visible and it needs to be default visible to match the original C code, yes?

@@ +18,5 @@
> +# stack.
> +  mov r12, rdi
> +  mov r13, rsi
> +
> +  # allocate space for stack arguments, in theory we only need 8 * (argv - 5)

What is argv here?  Also, the beginning of this line should start in the first column for consistency.

@@ +25,5 @@
> +# pointer specially.
> +  lea eax, [edx * 8]
> +  sub rsp, rax
> +
> +# if there is an odd number of args the stack can be missaligned so realign it.

Nit: "misaligned"

@@ +31,5 @@
> +
> +# pass the stack slot area to InvokeCopyToStack.
> +  mov r8, rsp
> +
> +# setup space for the register slots, there are 5 integer ones and 8 floating

Nit: ":" instead of "," here.

@@ +41,5 @@
> +# second is the floating point area.
> +  mov rdi, rsp
> +  lea rsi, [rsp + 40]
> +
> +# the 3rd and 4th arguments are our 3rd and 4th arguments so now we can just

"the 3rd and 4th arguments are our 3rd and 4th arguments"...tautological statement is tautological.

Maybe you meant: "the 3rd and 4th arguments to InvokeCopyToStack are already in the correct argument registers"?

@@ +48,5 @@
> +
> +# setup this
> +  mov rdi, r12
> +
> +#copy the integer arguments into place.

Nit: space between "#" and "copy"

::: xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp
@@ +7,5 @@
>  // Platform specific code to invoke XPCOM methods on native objects
>  
>  #include "xptcprivate.h"
>  
> +// 6 integral parameters are passed in registers, but 1 is this which isn't

I think we should say |this| to set it off as a clearly code |this| and not an english "this".

Comment 13

[Trevor Saunders (:tbsaunde)]

> @@ +2,5 @@
> > +
> > +# nsresult NS_InvokeByIndex(nsISupports* this, uint32_t aVtableIndex,
> > +#                           uint32_t argc, nsXPTCVariant* argv);
> > +.text
> > +.global NS_InvokeByIndex
> 
> This symbol is default visible and it needs to be default visible to match
> the original C code, yes?

yes, that and the declaration in xptcall.h which is XPCOM_API.  Might be nice to fix, but seems like it should at least be a separate patch.

> @@ +18,5 @@
> > +# stack.
> > +  mov r12, rdi
> > +  mov r13, rsi
> > +
> > +  # allocate space for stack arguments, in theory we only need 8 * (argv - 5)
> 
> What is argv here?  Also, the beginning of this line should start in the
> first column for consistency.

should have been argc, refering to the commented name for our third argument in the prototype.  Does it make more sense without the typo?

> @@ +41,5 @@
> > +# second is the floating point area.
> > +  mov rdi, rsp
> > +  lea rsi, [rsp + 40]
> > +
> > +# the 3rd and 4th arguments are our 3rd and 4th arguments so now we can just
> 
> "the 3rd and 4th arguments are our 3rd and 4th arguments"...tautological
> statement is tautological.

I should have written the callees 3rd and...

> Maybe you meant: "the 3rd and 4th arguments to InvokeCopyToStack are already
> in the correct argument registers"?

basically though I was trying to also explain why that is.

Comment 14

[Trevor Saunders (:tbsaunde)]

Attachment: rewrite NS_InvokeByIndex in assembly

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 8772831 [details] [diff] [review]
rewrite NS_InvokeByIndex in assembly

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

r=me with the below fixed.

::: xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.s
@@ +26,5 @@
> +# allocate space for stack arguments, in theory we only need 8 * (argc - 5)
> +# bytes because at least 5 arguments will go in registers, but for now it is
> +# just simpler to allocate 8 * (argc - 1) bytes.  Note that we treat the this
> +# pointer specially.
> +  lea eax, [edx * 8]

This doesn't actually allocate 8 * (argc - 1) bytes, though?  Either the comment or the code should be fixed.

@@ +30,5 @@
> +  lea eax, [edx * 8]
> +  sub rsp, rax
> +
> +# if there is an odd number of args the stack can be misaligned so realign it.
> +  and rsp, 0xffffffffffffff00

Just noticed this, but you are aligning the stack to 256 bytes here, not 16.  Please fix that, as I'm sure problems can result from this.

@@ +46,5 @@
> +  mov rdi, rsp
> +  lea rsi, [rsp + 40]
> +
> +# The 3rd and 4th arguments to InvokeCopyToStack are already in the right
> +# registers because they are our 3rd and 4th arguments.  So now we can just

Please drop "because they are our 3rd and 4th arguments", which still seems tautological.

Comment 16

[Trevor Saunders (:tbsaunde)]

> ::: xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.s
> @@ +26,5 @@
> > +# allocate space for stack arguments, in theory we only need 8 * (argc - 5)
> > +# bytes because at least 5 arguments will go in registers, but for now it is
> > +# just simpler to allocate 8 * (argc - 1) bytes.  Note that we treat the this
> > +# pointer specially.
> > +  lea eax, [edx * 8]
> 
> This doesn't actually allocate 8 * (argc - 1) bytes, though?  Either the
> comment or the code should be fixed.

Thinking about it now I'm not actually sure why I didn't just do (argc - 5) * 8, that seems easy enough, but for now it seems safer to just fix the comment and we can think about it more if there's some reason to care about the stack usage.

> @@ +30,5 @@
> > +  lea eax, [edx * 8]
> > +  sub rsp, rax
> > +
> > +# if there is an odd number of args the stack can be misaligned so realign it.
> > +  and rsp, 0xffffffffffffff00
> 
> Just noticed this, but you are aligning the stack to 256 bytes here, not 16.
> Please fix that, as I'm sure problems can result from this.

easy to miss, but wow that's certainly a waste of space.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e8cfd225f04
rewrite NS_InvokeByIndex in assembly r=froydnj

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out for OSX bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=32455543&repo=mozilla-inbound#L16638

08:25:00     INFO -      StaticXULComponentsEnd/StaticXULComponentsEnd.o
08:25:00     INFO -  ld: warning: could not create compact unwind for _ffi_call_unix64: does not use RBP or RSP based frame
08:25:00     INFO -  Undefined symbols for architecture x86_64:
08:25:00     INFO -    "_NS_InvokeByIndex", referenced from:
08:25:00     INFO -        __ZL16kFrozenFunctions in Unified_cpp_xpcom_build0.o
08:25:00     INFO -        __ZN16XPCWrappedNative10CallMethodER14XPCCallContextNS_8CallModeE in Unified_cpp_js_xpconnect_src1.o
08:25:00     INFO -        __ZN28txXPCOMExtensionFunctionCall8evaluateEP14txIEvalContextPP13txAExprResult in Unified_cpp_dom_xslt_xpath2.o
08:25:00     INFO -  ld: symbol(s) not found for architecture x86_64
08:25:00     INFO -  clang-3.8: error: linker command failed with exit code 1 (use -v to see invocation)

Comment 19

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86816e25dea7
Backed out changeset 2e8cfd225f04 for OSX bustage on a CLOSED TREE.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

FWIW, clobbering didn't help.

Comment 21

[Trevor Saunders (:tbsaunde)]

Attachment: compile xptcinvoke_asm_x86_64_unix.s when targeting darwin

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3114c70baccf
rewrite NS_InvokeByIndex in assembly r=froydnj

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f517b043170b
avoid clang's integrated assembler in the hope clang won't preprocess xptcinvoke_asm_x86_64_unix.s r=me landed on a CLOSED TREE

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for build failures on OS X (also on tier 1):

https://hg.mozilla.org/integration/mozilla-inbound/rev/4170491c6cbb3e8ae0586aefcf35e21c5e551b23
https://hg.mozilla.org/integration/mozilla-inbound/rev/c66bd05c9f55c22291eb60852a7224ea7bb16ff4

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f517b043170be2894efe648ca6247d05cdfc4f67
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=34579375&repo=mozilla-inbound

Comment 25

[Mike Hommey [:glandium]]

> rewrite NS_InvokeByIndex in assembly

What was the motivation for this change?

Comment 26

[Trevor Saunders (:tbsaunde)]

(In reply to Mike Hommey [:glandium] from comment #25)
> > rewrite NS_InvokeByIndex in assembly
> 
> What was the motivation for this change?

a couple things, the most important of which is that the old code was really hacky, and has kept needing hacks to keep compilers from misoptimizing it.  It was also really tricky and hard to understand how it works, and as suprising as it is the new code is shorter, and at least in my book easier to understand.  Its also faster according to TestXPCCall, which I've somewhat gtestified localy, but I don't really care about that, I think at this point we've gotten this out of any performance critical paths.

Do you have an objection, or why are you asking?

Comment 27

[Georg Koppen]

FWIW we'd like to get a fix into ESR52 as our hardened-builds would be otherwise broken. Not sure how we can help here, though, as at least my assembly skills are not good enough to move this forward.

Comment 28

[Trevor Saunders (:tbsaunde)]

(In reply to Georg Koppen from comment #27)
> FWIW we'd like to get a fix into ESR52 as our hardened-builds would be

I'll try to get this landed before 52 branches.  It shouldn't be hard, but I've got higher priority stuff to do at the moment, sorry.

> otherwise broken. Not sure how we can help here, though, as at least my
> assembly skills are not good enough to move this forward.

fwiw I don't the it should take much assembly skill, the code itself should be fine on mac which is where the problems are.  The problems I'm aware of are that the OSX assembler doesn't like the gas noexec stack stuff, which I believe you can just ifdef away there.  The other problem is that on darwin symbols have a "_" prefix so instead of NS_InvokeByIndex the assembly should contain _NS_InvokeByIndex and similar for the helper we call.  It should be pretty straight forward to fix that with preprocesser goo as well.  Since this file will then need preprocessing we should probably rename it to .S instead of .s if I remember the rules there correctly.

Comment 29

[Mike Hommey [:glandium]]

or, you know, just go with the original patch, which solves the original problem without turning it in "rewrite the whole thing in assembly".

Comment 30

[Trevor Saunders (:tbsaunde)]

Attachment: make darwin build

I'll fold this into the first patch to land, but its almost certainly easier to review separately.  This appears to be fine on try  all the orange in https://treeherder.mozilla.org/#/jobs?repo=try&revision=a83379d665d9eefc6df976bddf46b6edd953f93a looks random to me.

A couple things are needed, rename to .S and macro in a leading '_' on Darwin, and ifdef out .type and noexec stuff on Darwin.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/166775f0ea6c
rewrite NS_InvokeByIndex in assembly r=froydnj

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb3cad552a5f
remove a obsolete and now useless comment

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/166775f0ea6c
https://hg.mozilla.org/mozilla-central/rev/fb3cad552a5f

Comment 34

[Georg Koppen]

Hm, this missed mozilla52 by two days it seems. Trevor, do you think we could get that uplifted to aurora? The next Tor Browser will be based on ESR52 and having two patches less to juggle would already help us quite a bit.

Comment 35

[Trevor Saunders (:tbsaunde)]

(In reply to Georg Koppen from comment #34)
> Hm, this missed mozilla52 by two days it seems. Trevor, do you think we
> could get that uplifted to aurora? The next Tor Browser will be based on
> ESR52 and having two patches less to juggle would already help us quite a
> bit.

sorry about that.  Its not really the safest thing in the world, and its pretty core code, but on the other hand it should be well tested.  So I guess we can try though I'll leave the real call to froydnj and release managers.  Also aurora just branched and its supposed to be a long release so perhaps not that big a deal to backport stuff now.

Comment 36

[Mike Hommey [:glandium]]

If, as per comment 29, the original bug had been fixed/worked around instead of a full rewrite, uplifting would have been a no-brainer.

Comment 37

[Georg Koppen]

(In reply to Trevor Saunders (:tbsaunde) from comment #35)
> (In reply to Georg Koppen from comment #34)
> > Hm, this missed mozilla52 by two days it seems. Trevor, do you think we
> > could get that uplifted to aurora? The next Tor Browser will be based on
> > ESR52 and having two patches less to juggle would already help us quite a
> > bit.
> 
> sorry about that.  Its not really the safest thing in the world, and its
> pretty core code, but on the other hand it should be well tested.  So I
> guess we can try though I'll leave the real call to froydnj and release
> managers.  Also aurora just branched and its supposed to be a long release
> so perhaps not that big a deal to backport stuff now.

Thanks! froydnj what do you think given the arguments tbsaunde made?

Comment 38

[Nathan Froyd [:froydnj]]

(In reply to Georg Koppen from comment #37)
> (In reply to Trevor Saunders (:tbsaunde) from comment #35)
> > (In reply to Georg Koppen from comment #34)
> > > Hm, this missed mozilla52 by two days it seems. Trevor, do you think we
> > > could get that uplifted to aurora? The next Tor Browser will be based on
> > > ESR52 and having two patches less to juggle would already help us quite a
> > > bit.
> > 
> > sorry about that.  Its not really the safest thing in the world, and its
> > pretty core code, but on the other hand it should be well tested.  So I
> > guess we can try though I'll leave the real call to froydnj and release
> > managers.  Also aurora just branched and its supposed to be a long release
> > so perhaps not that big a deal to backport stuff now.
> 
> Thanks! froydnj what do you think given the arguments tbsaunde made?

I don't have a problem with uplifting this patch.

Comment 39

[Georg Koppen]

(In reply to Nathan Froyd [:froydnj] from comment #38)
> (In reply to Georg Koppen from comment #37)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #35)
> > > (In reply to Georg Koppen from comment #34)
> > > > Hm, this missed mozilla52 by two days it seems. Trevor, do you think we
> > > > could get that uplifted to aurora? The next Tor Browser will be based on
> > > > ESR52 and having two patches less to juggle would already help us quite a
> > > > bit.
> > > 
> > > sorry about that.  Its not really the safest thing in the world, and its
> > > pretty core code, but on the other hand it should be well tested.  So I
> > > guess we can try though I'll leave the real call to froydnj and release
> > > managers.  Also aurora just branched and its supposed to be a long release
> > > so perhaps not that big a deal to backport stuff now.
> > 
> > Thanks! froydnj what do you think given the arguments tbsaunde made?
> 
> I don't have a problem with uplifting this patch.

Thanks! Trevor, may I ask you to file the uplift request? I am not sure if I get all the details right and whether that should be done by a casual bug reporter anyway...

Comment 40

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8810285 [details] [diff] [review]
make darwin build

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:tor builds with gcc and asan don't build
[Describe test coverage new/current, TreeHerder]: its heavily used code, so should be fairly well tested, and hasn't shown any problems while on m-c for several days.
[Risks and why]: moderate, significant change, but should be very well tested.
[String/UUID change made/needed]:none

Comment 41

[Mike Hommey [:glandium]]

Note the keyword is /should/. We've had breakages from xptc assembly code changes that went unnoticed in the past.

Comment 42

[Julien Cristau [:jcristau]]

(In reply to Trevor Saunders (:tbsaunde) from comment #40)
> Comment on attachment 8810285 [details] [diff] [review]
> make darwin build
> 
> Approval Request Comment
> [Feature/regressing bug #]:
> [User impact if declined]:tor builds with gcc and asan don't build
> [Describe test coverage new/current, TreeHerder]: its heavily used code, so
> should be fairly well tested, and hasn't shown any problems while on m-c for
> several days.
> [Risks and why]: moderate, significant change, but should be very well
> tested.
> [String/UUID change made/needed]:none

As glandium says, attachment #8764936 [details] [diff] [review] seems like a safer candidate for this, if the goal is to help tor out for esr52...

Comment 43

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8810285 [details] [diff] [review]
make darwin build

Happy to help our tor friends, taking it to aurora.

Comment 44

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8810285 [details] [diff] [review]
make darwin build

Reverting my approval, I was testing a tool.

Comment 45

[Julien Cristau [:jcristau]]

Comment on attachment 8810285 [details] [diff] [review]
make darwin build

As I said in comment 42 I'm not keen on taking this rewrite for fx 52.

Comment 46

[Sylvestre Ledru [:Sylvestre]]

Looks like it is a wontfix.