[Lldb-commits] [PATCH] D24255: Fix for rL280668, Intel(R) Memory Protection Extensions (Intel(R) MPX) support.

[Pavel Labath via lldb-commits]

labath added inline comments.

================
Comment at: packages/Python/lldbsuite/test/functionalities/register/intel_xtended_registers/Makefile:5
@@ +4,3 @@
+
+ifeq "$(ARCH)" "i386"
+	CXXFLAGS += -mmpx -fcheck-pointer-bounds -fuse-ld=bfd -m32
----------------
valentinagiusti wrote:
> labath wrote:
> > This should not be necessary. Makefile.rules already correctly appends -m32 when needed. Maybe CFLAGS_EXTRAS would work instead (?)
> 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!
> 
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. 

================
Comment at: packages/Python/lldbsuite/test/functionalities/register/register_command/TestRegisters.py:297
@@ -294,4 +296,3 @@
             for registerSet in registerSets:
-                if 'advanced vector extensions' in registerSet.GetName().lower():
-                    has_avx = True
-                    break
+                if registerSet.GetName():
+                    if 'advanced vector extensions' in registerSet.GetName().lower():
----------------
valentinagiusti wrote:
> labath wrote:
> > Do we want to allow a register set with no name? It looks like the root of the problem is elsewhere.
> 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.
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.


https://reviews.llvm.org/D24255