Skip to content
This repository has been archived by the owner on Jan 27, 2019. It is now read-only.

Support address space beyond 24 bit on powerpc32 #58

Closed

Conversation

ksorensen
Copy link
Contributor

Enable support for larger address space on powerpc32.

Internal references will be resolved with 24 bit addressing if
possible, but external references will cause 32 bit addressing
to be used.

There is some performance impact, but this is necessary to
support large libraries like e.g. qt.

Signed-off-by: Klaus Henning Sorensen klaus.sorensen@prevas.dk

@esben esben added the ready label Oct 14, 2015
@esben esben added this to the 4.0.1 milestone Oct 14, 2015
# space larger than 24 bits
CFLAGS_OPT_SPEED:>TARGET_CPU_powerpc += " -fPIC"
CFLAGS_OPT_SIZE:>TARGET_CPU_powerpc += " -fPIC"
CFLAGS_OPT_DEBUG:>TARGET_CPU_powerpc += " -fPIC"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be adding it to these CFLAGS_OPT_* variables.

  1. They are used for both build, host and target CFLAGS. So you will be adding -fPIC to HOST_CFLAGS for both cross and sdk-cross, which I don't think was the idea.
  2. Users/developers are supposed to be overriding CFLAGS_OPT_SPEED, so it probably best to not piggy-back stuff to them.

Maybe do

HOST_CFLAGS:>HOST_CPU_powerpc += " -fPIC"
TARGET_CFLAGS:>TARGET_CPU_powerpc += " -fPIC"

instead. I haven't tried it, so some amount of testing is clearly a good idea ;)

@ksorensen
Copy link
Contributor Author

I can test this.

I think we should add a use flag to enable this instead of checking CPU_powerpc.

How about

USE_powerpc32_32bit_addr

@esben
Copy link
Contributor

esben commented Oct 16, 2015

As USE flag sounds good. But I think we should still limit the change to powerpc builds (ie. powerpc CFLAGS). For that, you need the *_CPU_powerpc overrides.

@ksorensen
Copy link
Contributor Author

What is the easy way of testing for two conditions in a conditional append - this is hopefully not the easiest way:

X = "file://powerpc_fPIC.patch;striplevel=2"
Y = ""
Z = ""
Y:TARGET_CPU_powerpc = "${X}"
Z:USE_powerpc32_32bit_addr = "${Y}"

SRC_URI:>USE_powerpc32_32bit_addr = "${Z}"

@esben
Copy link
Contributor

esben commented Oct 16, 2015

Actually, I don't think we have a good way to do this in OE-lite. For what it is worth, I have a simple and intuitive way to do that in XD-build, where it could be done as

SRC_URI.append_if(TARGET_CPU=="powerpc" and USE_powerpc_32bit_addr,
                  "file://powerpc_fPIC.patch;striplevel=2")

but this clearly does not help you now.

I'm afraid you will have to do something like what you suggested yourself :( Or perhaps a bit simpler:

SRC_URI:>USE_powerpc32_32bit_addr = "${SRC_URI_powerpc_32bit_addr}"
SRC_URI_powerpc_32bit_addr = ""
SRC_URI_powerpc_32bit_addr:TARGET_CPU_powerpc = " file://powerpc_fPIC.patch;striplevel=2"

@ksorensen
Copy link
Contributor Author

What do you say to something like this?

TARGET_CPU_powerpc ?= "0"
USE_powerpc32_32bit_addr ?= "0"
powerpc32_and_32bit = "@'1' if '${TARGET_CPU_powerpc}' != '0' and '${USE_powerpc32_32bit_addr}' != '0' else '0'"
SRC_URI:>powerpc32_and_32bit = "file://powerpc_fPIC.patch;striplevel=2"

@esben
Copy link
Contributor

esben commented Oct 16, 2015

That won't work. In OE-lite, the currently active overrides are defined as the colon separated list in ${OVERRIDES}. You would therefore need to append to that instead of definining a new variable.

But the 3 line example I gave above is probably as simple as it gets in OE-lite.

@esben
Copy link
Contributor

esben commented Oct 16, 2015

Also, FYI the inline OE-lite python syntax is "${@...}"

@ksorensen
Copy link
Contributor Author

I have force pushed a new attempt at this. It builds and works for me.

@@ -38,13 +38,13 @@ SDK_CXXFLAGS ?= "${SDK_CFLAGS} -fpermissive"
CXXFLAGS = "${HOST_CXXFLAGS}"
HOST_CXXFLAGS:native = "${BUILD_CXXFLAGS}"
HOST_CXXFLAGS:cross = "${BUILD_CXXFLAGS}"
HOST_CXXFLAGS:machine = "${MACHINE_CXXFLAGS}"
HOST_CXXFLAGS:machine = "${MACHINE_CXXFLAGS} ${PIC_OPTION}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you are looking at TARGET_CPU_powerpc to control PIC_OPTION, so adding to HOST_CXXFLAGS does not look right.

@ksorensen
Copy link
Contributor Author

I have some trouble determining exactly where the ${PIC_OPTION} is needed.

Now it has been added to HOST_.*FLAGS:machine and TARGET_.*FLAGS:machine.

Am I correct in assuming it should be removed from HOST_.*FLAGS:machine and only left in for TARGET_.*FLAGS:machine?

@esben
Copy link
Contributor

esben commented Oct 21, 2015

I believe we need something like

PIC_OPTION_powerpc_32bit_addr=""
PIC_OPTION_powerpc:USE_powerpc_32bit_addr="-fPIC"
HOST_PIC_OPTION=""
HOST_PIC_OPTION:HOST_CPU_powerpc = "${PIC_OPTION_powerpc_32bit_addr}"
TARGET_PIC_OPTION=""
TARGET_PIC_OPTION:HOST_CPU_powerpc = "${PIC_OPTION_powerpc_32bit_addr}"

and then add HOST_PIC_OPTION to HOST_CXXFLAGS:machine, HOST_CPPFLAGS:machine and HOST_CFLAGS:machine, and TARGET_PIC_OPTION to HOST_CXXFLAGS:machine and so on.

I don't think the _OPT flags should be touched.

I totally agrees that this flag C* handling has gotten out of control. Sorry about that :(
But in this case, we do need to handle both host and target flags, as machine recipes have both HOST and TARGET as powerpc when MACHINE is powerpc.

@ksorensen
Copy link
Contributor Author

I tried building without setting -fPIC on the HOST flags. It is not sufficient, the build does not work.

What is the reason you would prefer not to set -fPIC on the _OPT flags? If -fPIC is not set, the binary will not work if a relative branch in the runtime linked code exceeds 24bit in size.

@esben
Copy link
Contributor

esben commented Oct 22, 2015

I don't think -fPIC belongs in *_CFLAGS_OPT variables, as they are dedicated to optimization, and the idea is that it should be perfectly fine for recipes to override these as needed to workaround problems with optimization levels. Adding something important like this to these variables break that.

I am not sure exactly what you mean with "I tried building without setting -fPIC on the HOST flags".

We definitely do need -fPIC on HOST flags. But only when HOST_CPU_powerpc override is enabled.

When building fx. cross:gcc (arch tuple build/build/machine), you should only have -fPIC in HOST flags.
When building fx. machine:eglibc (arch tuple build/machine/machine), you should have -fPIC in both HOST and TARGET flags.

If you add it to both HOST and TARGET flags based on TARGET_CPU_powerpc, you will add -fPIC to CFLAGS for the HOST (== BUILD) when you are building a powerpc cross compiler, but not when you are building an arm cross compiler. That seems wrong to me.

@ksorensen
Copy link
Contributor Author

I have started a build based on your suggestion with

PIC_OPTION_powerpc                             = ""
PIC_OPTION_powerpc:USE_powerpc_32bit_addr      = "-fPIC"
HOST_PIC_OPTION                                = ""
HOST_PIC_OPTION:HOST_CPU_powerpc               = "${PIC_OPTION_powerpc}"
TARGET_PIC_OPTION                              = ""
TARGET_PIC_OPTION:TARGET_CPU_powerpc           = "${PIC_OPTION_powerpc}"

and

HOST_CFLAGS:machine             = "${MACHINE_CFLAGS} ${HOST_PIC_OPTION}"
...
TARGET_CFLAGS:machine           = "${MACHINE_CFLAGS} ${TARGET_PIC_OPTION}"

and have removed *_PIC_OPTION from the *CFLAGS_OPT options.

@esben
Copy link
Contributor

esben commented Oct 22, 2015

I guess you also still appends to HOST_CXXFLAGS and TARGET_CXXFLAGS?

Enable support for larger address space on powerpc32.

Internal references will be resolved with 24 bit addressing if
possible, but external references will cause 32 bit addressing
to be used.

There is some performance impact, but this is necessary to
support large libraries like e.g. qt.

Add use flag

	USE_powerpc_32bit_addr

When set and compiling for powerpc, will add -fPIC to
enable 32 bit branch addresses.

Signed-off-by: Klaus Henning Sorensen <klaus.sorensen@prevas.dk>
@ksorensen ksorensen force-pushed the 4.0_powerpc32_32bit_addressing branch from cb97282 to e6f3f98 Compare October 26, 2015 14:11
@ksorensen
Copy link
Contributor Author

Force pushed new version.

Use the same powerpc_32bit_addr use flag for both -fPIC options when building and applying patch to eglibc startup code.

Separate HOST_PIC_OPTION and TARGET_PIC_OPTION based on HOST_CPU_powerpc and TARGET_CPU_powerpc.

Only add -fPIC to _CFLAGS, _CXXFLAGS and _CPPFLAGS - not added to CFLAGS_OPTS.

Project has been built and the resulting binaries work correctly on target hardware

@@ -4,6 +4,8 @@
## machine, sdk, cross, sdk-cross, canadian-cross (=all). A similar class
## exists for c++, see the c++ class.

CLASS_FLAGS:>machine = " powerpc_32bit_addr"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should probably be a '+=' in case it is combined with another class that uses CLASS_FLAGS:>machine.

@esben
Copy link
Contributor

esben commented Oct 26, 2015

I know it has been a loooooong process, but if you could split the eglibc recipe changes into a separate commit (keep in this PR), that would be nice. That way I can easily cherry-pick the other part for master branch.

You also need to rebase on top of current master.

@ksorensen
Copy link
Contributor Author

I had hoped to have a word with you yesterday about this, but we spent the time on other stuff.

I have to pause the effort on this for the moment - I have run out of time and testing small changes to these recipes on which everything depend on takes a long time.

There is a small issue regarding splitting the patch up in two

Right now, both the eglibc and c[++].oeclass depend on the same use flag - USE_powerpc_32bit_addr.

I had

RECIPE_FLAGS:machine    += " powerpc_32bit_addr"

in the eglibc recipe and

CLASS_FLAGS:>machine    = " powerpc_32bit_addr"

in c.oeclass.

This caused the fPIC patch to be applied twice in the eglibc recipe, probably because powerpc_32bit_addr was seen twice by the eglibc recipe (once from c.oeclass and once from the recipe itself) and as a side effects of how variables are evaluated and expanded.

I removed the explicit declaration of the use flag from the eglibc recipe, and things work as expected.

What I am getting at is, that if the patch is split into two, the declaration of the use flag will have to be added to the eglibc patch, and removed again when both patches are applied. As it stands now, these two changes (eglibc and c.oeclass) affect each other.

@esben
Copy link
Contributor

esben commented Oct 29, 2015

I have split the commit (not the PR), rebased to upstream/4.0 and created a new PR.
@ksorensen please review, and let me know if it is okay to merge. I have kept your name on the commit, so you are still responsible ;)

@esben esben closed this Oct 29, 2015
@esben esben removed the ready label Oct 29, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants