New issue
Advanced search Search tips

Issue 882085 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 21
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android
Pri: 3
Type: Bug



Sign in to add a comment

Remove __mips32__ and __mips64__ defines from base/macros.h

Project Member Reported by thestig@chromium.org, Sep 8

Issue description

r502712 added __mips32__ and __mips64__ defines in base/macros.h. Why is this needed there? Why can't ARCH_CPU_MIPS_FAMILY and friends from build/build_config.h be used instead?
 
Cc: palmer@chromium.org
Labels: OS-Android
Cc: -wangqing...@loongson.com wangqing...@loongson.cn
Are there instructions somewhere for how to build for MIPS from Linux or macOS? If yes, I can try to help. Otherwise I can't because I will be working blindly and will likely just end up breaking the MIPS build.
This is the configuration that I usually use:

mipsel: gn gen out/Release --args=" target_os=\"linux\" target_cpu=\"mipsel\" mips_arch_variant=\"r2\" is_debug=false"

mips64el: gn gen out/Release --args=" target_os=\"linux\" target_cpu=\"mips64el\" mips_arch_variant=\"r2\" is_debug=false"
What is the reason for removing these defines? 

Maybe I can work on a different solution.
We already have a way to define CPU architecture in build/build_config.h. I don't believe it is necessary for MIPS to have its own separate way of doing something similar in base/macros.h.
re: comment 5 - Thanks! Is there a quick guide to building for MIPS somewhere, BTW? Looks like I also have to run: build/linux/sysroot_scripts/install-sysroot.py --arch=mipsel, but otherwise it is building...
Cc: jorgelo@chromium.org
+jorgelo from sandbox/linux/OWNERS for discussion.

It looks like __mips32__ and __mips64__ are only used in sandbox/linux, where they want to fit in with __i386__ and __x86_64__. The issue is, __x86_64__ is defined by the compiler, but __mips64__ is not defined.

So one possible solution is to switch sandbox/linux to use the defines from build/build_config.h, to be consistent with the rest of the code base. It will have to be a bit more verbose though. e.g.

#if defined(__i386__) || defined(__x86_64__) || defined(__mips32__)

becomes:

#if defined(ARCH_CPU_X86_FAMILY) || (defined(ARCH_CPU_MIPS_FAMILY) && defined(ARCH_CPU_32_BITS))

Would that work?


Another idea is to move __mips32__ and __mips64__ into a header in sandbox/linux.
Yes, #if (defined(ARCH_CPU_MIPS_FAMILY) && defined(ARCH_CPU_32_BITS)) will work.

Let me know if you want us to upload the change.
Re: Comment 7: these are all the steps:

mips32el:
Get the sysroot:
$ build/linux/sysroot_scripts/install-sysroot.py --arch=mipsel 

Configure and build:
$ gn gen out/Release --args=" target_os=\"linux\" target_cpu=\"mipsel\" mips_arch_variant=\"r2\" is_debug=false"
$ ninja -C out/Release chrome -j6

mips64el:
Get the sysroot:
$ build/linux/sysroot_scripts/install-sysroot.py --arch=mips64el 

Configure and build:
$ gn gen out/Release --args=" target_os=\"linux\" target_cpu=\"mips64el\" mips_arch_variant=\"r2\" is_debug=false"
$ ninja -C out/Release chrome -j6
I'm fine with switching the defines to the ones from build/build_config.h, as long as they mean the same thing.
What's the status here?
Someone needs to do a lot of search and replace. e.g. s/__x86_64__/ARCH_CPU_X86_64/ and s/__i386__/ARCH_CPU_X86/
Wait, I thought we said __x86_64__ and __i386__ are fine because they're compiler-defined and we only need to replace __mips32__ and __mips64__?
Owner: thestig@chromium.org
Status: Assigned (was: Untriaged)
We did? Anyway, let's start with __mips32__ and __mips64__, so we can close out this bug. I'll take it.
Project Member

Comment 17 by bugdroid1@chromium.org, Dec 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e78585880cc5f3dc73411b71fb9c7e81f0615c4

commit 1e78585880cc5f3dc73411b71fb9c7e81f0615c4
Author: Lei Zhang <thestig@chromium.org>
Date: Fri Dec 21 22:58:50 2018

Remove __mips32__ and __mips64__ defines from base/macros.h.

BUG= 882085 

Change-Id: I07243c03a28a45f3d368269668b54eadc0b50e98
Reviewed-on: https://chromium-review.googlesource.com/c/1362223
Reviewed-by: Chris Palmer <palmer@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618650}
[modify] https://crrev.com/1e78585880cc5f3dc73411b71fb9c7e81f0615c4/base/macros.h
[modify] https://crrev.com/1e78585880cc5f3dc73411b71fb9c7e81f0615c4/sandbox/linux/bpf_dsl/linux_syscall_ranges.h
[modify] https://crrev.com/1e78585880cc5f3dc73411b71fb9c7e81f0615c4/sandbox/linux/bpf_dsl/seccomp_macros.h
[modify] https://crrev.com/1e78585880cc5f3dc73411b71fb9c7e81f0615c4/sandbox/linux/bpf_dsl/syscall_set.cc
[modify] https://crrev.com/1e78585880cc5f3dc73411b71fb9c7e81f0615c4/sandbox/linux/seccomp-bpf-helpers/baseline_policy.cc
[modify] https://crrev.com/1e78585880cc5f3dc73411b71fb9c7e81f0615c4/sandbox/linux/seccomp-bpf-helpers/baseline_policy_android.cc
[modify] https://crrev.com/1e78585880cc5f3dc73411b71fb9c7e81f0615c4/sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc
[modify] https://crrev.com/1e78585880cc5f3dc73411b71fb9c7e81f0615c4/sandbox/linux/seccomp-bpf-helpers/syscall_sets.cc
[modify] https://crrev.com/1e78585880cc5f3dc73411b71fb9c7e81f0615c4/sandbox/linux/seccomp-bpf-helpers/syscall_sets.h
[modify] https://crrev.com/1e78585880cc5f3dc73411b71fb9c7e81f0615c4/sandbox/linux/seccomp-bpf/syscall.cc
[modify] https://crrev.com/1e78585880cc5f3dc73411b71fb9c7e81f0615c4/sandbox/linux/syscall_broker/broker_process.cc
[modify] https://crrev.com/1e78585880cc5f3dc73411b71fb9c7e81f0615c4/sandbox/linux/system_headers/linux_signal.h
[modify] https://crrev.com/1e78585880cc5f3dc73411b71fb9c7e81f0615c4/sandbox/linux/system_headers/linux_syscalls.h

Status: Fixed (was: Assigned)
Project Member

Comment 19 by bugdroid1@chromium.org, Dec 22

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d6db857be00802afb220199f1792951b4d155b14

commit d6db857be00802afb220199f1792951b4d155b14
Author: Lei Zhang <thestig@chromium.org>
Date: Sat Dec 22 06:07:14 2018

Fix bad __mips64__ conversions from r618650.

!defined(__mips64__) got mechanically converted to:
!defined(ARCH_CPU_MIPS_FAMILY) && defined(ARCH_CPU_64_BITS)
when it should have been:
!(defined(ARCH_CPU_MIPS_FAMILY) && defined(ARCH_CPU_64_BITS))

BUG= 882085 
TBR=palmer@chromium.org

Change-Id: I1a21803da3d72965fa3ab113a9e54005c1d33e93
Reviewed-on: https://chromium-review.googlesource.com/c/1389796
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618734}
[modify] https://crrev.com/d6db857be00802afb220199f1792951b4d155b14/sandbox/linux/seccomp-bpf-helpers/syscall_sets.cc

Sign in to add a comment