Closed
Bug 1305396
Opened 8 years ago
Closed 7 years ago
mingw-w64 based on GCC >= 5.4.0 fails with "error: 'memmove' was not declared in this scope"
Categories
(Core :: Security: PSM, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: gk, Assigned: boklm)
References
(Blocks 1 open bug)
Details
(Whiteboard: [psm-assigned][tor])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
I finally got a cross-compiler based on GCC 6.2.0 built. But that one breaks compiling Firefox (at least esr45) with
/home/ubuntu/build/tor-browser/security/pkix/lib/pkixnames.cpp: In function 'bool mozilla::pkix::{anonymous}::FinishIPv6Address(uint8_t (&)[16], int, int)':
/home/ubuntu/build/tor-browser/security/pkix/lib/pkixnames.cpp:1701:32: error: 'memmove' was not declared in this scope
componentsToMove * 2u);
^
make[5]: *** [pkixnames.o] Error 1
Reporter | ||
Comment 1•8 years ago
|
||
Jacek, back in bug 1199624 you fixed the issues with memcmp and memset. Were there any particular reasons to omit memmove?
Flags: needinfo?(jacek)
Updated•8 years ago
|
Component: Security → Security: PSM
Comment 2•8 years ago
|
||
I'm sorry for the delay. I don't remember details of this bug, but I'm pretty sure I simply had no reason to omit memmove as it didn't cause any problem.
I can't really test it now, because tip is badly broken for mingw by configure scripts rewrite :/
Flags: needinfo?(jacek)
Priority: -- → P5
Whiteboard: [psm-backlog]
Updated•8 years ago
|
Whiteboard: [psm-backlog] → [psm-backlog][tor]
Reporter | ||
Comment 3•8 years ago
|
||
I can actually hit this bug with GCC 5.4.0 already.
Summary: mingw-w64 based on GCC 6.2.0 fails with "error: 'memmove' was not declared in this scope" → mingw-w64 based on GCC >= 5.4.0 fails with "error: 'memmove' was not declared in this scope"
Reporter | ||
Comment 4•7 years ago
|
||
It seems this got fixed between ESR 45 and ESR 52. I thought about finding out what changeset did cause this, but unfortunately I did not have my bisect environment anymore with all the patches needed for dealing with issues between ESR 45 and ESR 52. Setting this up again is not worth the trouble for me. Thus, this is just a simple WORKSFORME. :(
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
Reporter | ||
Comment 5•7 years ago
|
||
(In reply to Georg Koppen from comment #4)
> It seems this got fixed between ESR 45 and ESR 52. I thought about finding
> out what changeset did cause this, but unfortunately I did not have my
> bisect environment anymore with all the patches needed for dealing with
> issues between ESR 45 and ESR 52. Setting this up again is not worth the
> trouble for me. Thus, this is just a simple WORKSFORME. :(
Seems I've been too fast. We are hitting this (again) when trying to build for 64-bit Windows (https://trac.torproject.org/projects/tor/ticket/23442).
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Assignee | ||
Comment 6•7 years ago
|
||
Here is a proposed patch for fixing that, similar to the fix for bug 1199624.
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8907854 -
Flags: review?(dkeeler)
Comment on attachment 8907854 [details] [diff] [review]
Replace memmove with std::copy_backward in a file that doesn't include cstring explicitly
Review of attachment 8907854 [details] [diff] [review]:
-----------------------------------------------------------------
Just a heads-up: Mozilla style is to include 8 lines of context in patches.
This looks good, but I have a simplifying change I think we should make. r- for now.
::: security/pkix/lib/pkixnames.cpp
@@ +1707,4 @@
> // Shift components that occur after the contraction over.
> size_t componentsToMove = static_cast<size_t>(numComponents -
> contractionIndex);
> + std::copy_backward(address + (2u * static_cast<size_t>(contractionIndex)),
Looks like this needs a #include <algorithm> in this file (the style is to list it before other #includes, with a blank line after it).
@@ +1707,5 @@
> // Shift components that occur after the contraction over.
> size_t componentsToMove = static_cast<size_t>(numComponents -
> contractionIndex);
> + std::copy_backward(address + (2u * static_cast<size_t>(contractionIndex)),
> + address + (2u * static_cast<size_t>(contractionIndex))
Actually, I think we can simplify this by getting rid of componentsToMove altogether and just using `address + (2u * static_cast<size_t>(numComponents))` here.
Attachment #8907854 -
Flags: review?(dkeeler) → review-
Assignee: nobody → boklm
Priority: P5 → P1
Whiteboard: [psm-backlog][tor] → [psm-assigned][tor]
Keywords: stale-bug
Updated•7 years ago
|
Assignee | ||
Comment 9•7 years ago
|
||
Assignee | ||
Comment 10•7 years ago
|
||
Thanks for the review and sorry for taking a long time to reply. I attached a new patch taking into account your comments.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad8684558b50382d90beaa35b420fe3af878c6ab
Assignee | ||
Updated•7 years ago
|
Attachment #8919135 -
Flags: review?(dkeeler)
Comment on attachment 8919135 [details] [diff] [review]
Replace memmove with std::copy_backward in a file that doesn't include cstring explicitly
Review of attachment 8919135 [details] [diff] [review]:
-----------------------------------------------------------------
Great - thanks.
Attachment #8919135 -
Flags: review?(dkeeler) → review+
Attachment #8907854 -
Attachment is obsolete: true
Priority: P3 → P1
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 12•7 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a5c3789fac1
Replace memmove with std::copy_backward in a file that doesn't include cstring explicitly. r=keeler
Keywords: checkin-needed
Comment 13•7 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•