[PATCH] D36823: [LLD][ELF] Read ARM BuildAttributes section to determine supported features.

[Peter Smith via Phabricator via llvm-commits]

peter.smith added inline comments.


================
Comment at: ELF/Config.h:112-114
+  bool ARMHasBlx = false;
+  bool ARMHasMovtMovw = false;
+  bool ARMJ1J2BranchEncoding = false;
----------------
emaste wrote:
> Should these now be `ArmHas...` etc.? (i.e., new capitalization as mentioned on llvm-weekly)
I haven't got a good answer for you unfortunately. The Arm form is correct according to the new capitalization, but ARM is more consistent with the names throughout the rest of the code. My guess was that consistency would be preferred although I'm very happy to change to use Arm.  


================
Comment at: ELF/Driver.cpp:1073-1075
+      Config->ARMHasBlx = true;
+      Config->ARMHasMovtMovw = true;
+      Config->ARMJ1J2BranchEncoding = true;
----------------
emaste wrote:
> As a general principle I'd think these should really default to false, but until lld can generate proper output for the false cases I'd say of course this is preferred. (Or, is it the case that we expect tool chains to always produce an attributes section?)
> 
> Once it can generate proper output (and tests cover both cases) and the warnings below disappear perhaps this case for lack of attribute sections can become a warning?
> 
> 
The build attributes section is not required by the ABI, however in practice all widely used ARM C/C++ compilers provide one as the intersection of the subsets of Arm/Thumb that are supported by all the different Arm CPUs is very restrictive. The assembler won't automatically generate a build attributes section unless it has at least one .eabi_attribute directive, although links using exclusively assembler  produced objects tend to be restricted to assembler and linker tests.

I think it would be worthwhile switching the defaults to false when more support for older Arm architectures is added, it would mean adding .eabi_attribute directives to some of the existing tests, but I don't think that this will be too much effort. I also agree a warning for no build attributes is sensible if the defaults go to false as it is usually a mistake by the user. 


https://reviews.llvm.org/D36823