Page MenuHomePhabricator

Fix for rL280668, Intel(R) Memory Protection Extensions (Intel(R) MPX) support.

Authored by valentinagiusti on Sep 6 2016, 7:35 AM.

Diff Detail


Event Timeline

valentinagiusti retitled this revision from to Fix for rL280668, Intel(R) Memory Protection Extensions (Intel(R) MPX) support..
valentinagiusti updated this object.
valentinagiusti added a subscriber: lldb-commits.
labath requested changes to this revision.Sep 6 2016, 8:03 AM
labath edited edge metadata.

I have a couple of questions I'd like to be answered before this goes in. Apart from the inline comments, all my questions from the previous version of this commit still stand.

5 ↗(On Diff #70398)

This should not be necessary. Makefile.rules already correctly appends -m32 when needed. Maybe CFLAGS_EXTRAS would work instead (?)

297 ↗(On Diff #70398)

Do we want to allow a register set with no name? It looks like the root of the problem is elsewhere.

This revision now requires changes to proceed.Sep 6 2016, 8:03 AM

Hi, inline there are my other replies.

5 ↗(On Diff #70398)

Unfortunately it doesn't append -m32 to all the instances when also a linker is needed in the build process. In fact, in the test logs it shows that only the first call of the g++ command has such a flag, and therefore the inferior code build ends with an error.
If there is a better way to do the same with CFLAGS_EXTRAS please let me know!

297 ↗(On Diff #70398)

These lines of code are just to detect if there are AVX or MPX register sets, so I don't think there is the need to do anything about nameless sets here. If you don't like this solution, I think an alternative is to just check if there are the register names that belong to one set or the other, it just takes a bit longer - or I could just look for the first register in the set.

clayborg requested changes to this revision.Sep 6 2016, 9:43 AM
clayborg edited edge metadata.

Just switch to using CFLAGS_EXTRAS and LD_EXTRAS and this is good to go.

6–11 ↗(On Diff #70398)

It would be nice to not modify the CXXFLAGS or LDFLAGS directly. You should modify CFLAGS_EXTRAS and LD_EXTRAS. I am not sure if there is a CXXFLAGS_EXTRAS, but feel free to add it if you can't use CFLAGS_EXTRAS

valentinagiusti edited edge metadata.

Improved and Makefile according to review.

labath added inline comments.Sep 7 2016, 6:18 AM
5 ↗(On Diff #70398)

I don't see how that is possible. I just made the following test:

$ cat Makefile
LEVEL = ../../../make

CXX_SOURCES := main.cpp

CFLAGS_EXTRAS += -Wzero-as-null-pointer-constant

include $(LEVEL)/Makefile.rules
 $ make
g++  -std=c++11  -g -O0 -fno-builtin -m64  -Wzero-as-null-pointer-constant -I/usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/make/../../../../../include -include /usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/make/test_common.h  -c -o main.o main.cpp
g++  main.o -g -O0 -fno-builtin -m64  -Wzero-as-null-pointer-constant -I/usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/make/../../../../../include -include /usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/make/test_common.h    -o "a.out"
$ make ARCH=i386
g++  -std=c++11  -g -O0 -fno-builtin -m32  -Wzero-as-null-pointer-constant -I/usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/make/../../../../../include -include /usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/make/test_common.h  -c -o main.o main.cpp
g++  main.o -g -O0 -fno-builtin -m32  -Wzero-as-null-pointer-constant -I/usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/make/../../../../../include -include /usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/make/test_common.h    -o "a.out"

As you see CFLAGS_EXTRAS was added to both compiler and linker command lines for both 32-bit and 64-bit cases (so, setting LD_EXTRAS is not necessary). Also, Makefile.rules already correctly appended the correct -m32/64 switch to all compiler and linker executions (so, you should not need to set it yourself).

If this does not work for you, then we need to figure out why and fix it. I don't believe we should be adding complex rules to these Makefiles, as it increases the maintenance burden down the line.

297 ↗(On Diff #70398)

I was referring to the addition of the if registerSet.GetName() pre-check. I know in the previous version of the commit this test failed here due to register set name being None. So this also implicitly checked that each register set has a name. Now, you've circumvented that check. This seems to be unnecessary because the test passes even when I remove that, so I think we should do that. I think it's a reasonable expectation that each register set has a name.

valentinagiusti marked an inline comment as done.Sep 7 2016, 7:13 AM
valentinagiusti added inline comments.
6 ↗(On Diff #70507)

You are right, if I use CFLAGS_EXTRAS I don't have to check for the arch or use LD_EXTRAS. I wish I knew this from the beginning...

clayborg accepted this revision.Sep 7 2016, 8:45 AM
clayborg edited edge metadata.
labath added a comment.Sep 7 2016, 9:42 AM

I think we're approaching the end now. If you could upload the new version (you'll have to reformat the changes), I'll give it a final pass.

6 ↗(On Diff #70507)

Yeah, I don't blame you. :) The test suite is not an easy thing to wrap your head around. I am sorry if some of my comments sounded a bit harsh.

valentinagiusti edited edge metadata.

Improved MPX test Makefile and removed workaround for unnamed register sets, and rebased according to the new coding style.

valentinagiusti added inline comments.Sep 8 2016, 7:06 AM
7 ↗(On Diff #70695)

Np, thanks for the review and explanations ;)

labath accepted this revision.Sep 8 2016, 7:10 AM
labath edited edge metadata.

LGTM, thanks.

This revision is now accepted and ready to land.Sep 8 2016, 7:10 AM
This revision was automatically updated to reflect the committed changes.