This is an archive of the discontinued LLVM Phabricator instance.

[LLD][ELF] Cortex-M Security Extensions (CMSE) Support
ClosedPublic

Authored by amilendra on Dec 1 2022, 2:57 AM.

Details

Summary

This commit provides linker support for Cortex-M Security Extensions (CMSE).
The specification for this feature can be found in ARM v8-M Security Extensions:
Requirements on Development Tools.

The linker synthesizes a security gateway veneer in a special section;
.gnu.sgstubs, when it finds non-local symbols __acle_se_<entry> and <entry>,
defined relative to the same text section and having the same address. The
address of <entry> is retargeted to the starting address of the
linker-synthesized security gateway veneer in section .gnu.sgstubs.

In summary, the linker translates input:

  .text
entry:
__acle_se_entry:
  [entry_code]

into:

  .section .gnu.sgstubs
entry:
  SG
  B.W __acle_se_entry

  .text
__acle_se_entry:
  [entry_code]

If addresses of __acle_se_<entry> and <entry> are not equal, the linker
considers that <entry> already defines a secure gateway veneer so does not
synthesize one.

If --out-implib=<out.lib> is specified, the linker writes the list of secure
gateway veneers into a CMSE import library <out.lib>. The CMSE import library
will have 3 sections: .symtab, .strtab, .shstrtab. For every secure gateway
veneer <entry> at address <addr>, .symtab contains a SHN_ABS symbol <entry> with
value <addr>.

If --in-implib=<in.lib> is specified, the linker reads the existing CMSE import
library <in.lib> and preserves the entry function addresses in the resulting
executable and new import library.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
MaskRay added inline comments.Mar 6 2023, 11:59 PM
lld/ELF/Arch/ARM.cpp
979

The two conditions can be combined as done in checkCmseSymAttributes. The message can be unified as well.

1025

The 3 checks can be made a local lambda to be shared by acleSeSym and sym. The lambda can have a parameter to differentiate special and entry.

1027

is not defined.

Again, Not a Defined does not imply Undefined.

1056

Printing an error for every symbol is verbose. I think armCMSESupport should be removed.

In updateSupportedARMFeatures, report a single error if not v8-M and perhaps config->cmseImplib = false; to disable processArmCmseSymbol.

1078
1125

wrong section name

1140

inline this used-once variable and delete braces

lld/ELF/Driver.cpp
2815
lld/ELF/InputFiles.cpp
336

Place the if (file) check in the caller and use InputFile &file instead of std::optional.

1164

revert

lld/ELF/LinkerScript.cpp
900
904
lld/ELF/SyntheticSections.cpp
3857

Move this before ppc64 to match the order in the declaration.

lld/ELF/SyntheticSections.h
1180

The inline doesn't seem useful. It's only called once anyway.

lld/ELF/Writer.cpp
457

Do this only if EM_ARM

604

Move the condition config->cmseOutputLib.empty() and then you can remove the EM_ARM condition.

lld/test/ELF/arm-cmse-check-startaddress.s
3 ↗(On Diff #502635)
9 ↗(On Diff #502635)

Use FileCheck /dev/null --implicit-check-not=error: in stead of NOERROR

lld/test/ELF/arm-cmse-check-sym-attrs.s
2 ↗(On Diff #502635)
lld/test/ELF/arm-cmse-cmdline-options.s
1 ↗(On Diff #502635)

There are still too many test files. Consider grouping this negative test with others together.

2 ↗(On Diff #502635)
amilendra updated this revision to Diff 503380.Mar 8 2023, 8:02 AM

Address further review comments.
Consolidate diagnostic tests to a single test file.

amilendra marked 2 inline comments as done.Mar 8 2023, 9:24 AM
amilendra added inline comments.
lld/ELF/Arch/ARM.cpp
1056

Moving the error to updateSupportedARMFeatures will cause the link to silently continue when build attributes are not found. I've changed in a way that processArmCmseSymbol reports the error once and then resetting config->cmseImplib so that further processing in processArmCmseSymbol is not done. Is this okay?

lld/test/ELF/arm-cmse-cmdline-options.s
1 ↗(On Diff #502635)

I've consolidated all diagnostics for the arm architecture to a single test file.

MaskRay added inline comments.Mar 8 2023, 10:51 AM
lld/ELF/Arch/ARM.cpp
1004

Merge isNotDefined/isNotThumbFunc/isAbsolute into one lambda.

1038
1129

Use emplace_back

1132

Move the comment before if (acleSeSym->file != sym->file || and delete braces

1161
1167

this-> can be removed. Applies elsewhere.

1172

Use .has_value() if you want to emphasize it is a std::optional.

1173

Delete blank line

1217

It's unclear what the +4 for p is and what the 4 in s - p - 4 is.

Perhaps just combine them into s - getVA() - 8 below.

1265

Why not merge the two osIsPairs loops?

1277

Remove used-once variable or add const if the use site(s) are far away.

1282

unsigned flags = config->mmapOutputFile ? 0 : (unsigned)FileOutputBuffer::F_no_mmap;

amilendra updated this revision to Diff 503724.Mar 9 2023, 3:51 AM
amilendra marked an inline comment as done.

Address another round of review comments.

amilendra marked 29 inline comments as done.Mar 9 2023, 4:01 AM
amilendra marked 18 inline comments as done.Mar 9 2023, 4:55 AM
amilendra marked an inline comment as done.Mar 9 2023, 5:08 AM
amilendra updated this revision to Diff 503742.Mar 9 2023, 5:52 AM

Address some review comments I missed.

amilendra marked 7 inline comments as done.Mar 9 2023, 5:55 AM
amilendra updated this revision to Diff 503746.Mar 9 2023, 6:07 AM

Use emplace_back() and remove explicit make_pair().

MaskRay accepted this revision.Mar 9 2023, 3:00 PM

Thanks! LGTM from my review. It'd be good to have a review from a domain expert.

I am going to be out for the next 4 weeks, and will likely have little time reading patches. The patch looks good to land when a domain expert thinks it's fine.

lld/ELF/Arch/ARM.cpp
1039
lld/ELF/InputFiles.cpp
336

My comment may be unclear.

if (armCmseImpLib) parseArmCMSEImportLib(*armCmseImpLib);, then this function will not need a std::optional argument.

lld/ELF/InputFiles.h
51

Did you mean: Parse symbols in an Arm CMSE import library?

Or perhaps just remove the comment since the function name is self-explanatory.

lld/ELF/Options.td
94

to the location specified => to <file>.

The metasyntactic name <file> can be used to simplify and clarify the help message.

Perhaps @peter.smith has some specific suggestions here.

This revision is now accepted and ready to land.Mar 9 2023, 3:00 PM
MaskRay added inline comments.Mar 9 2023, 3:02 PM
lld/ELF/Arch/ARM.cpp
1026

__acle_se_<sym>

amilendra updated this revision to Diff 504065.Mar 10 2023, 2:19 AM

Address review comments.

amilendra added a subscriber: psmith.EditedMar 10 2023, 2:29 AM

Thanks! LGTM from my review. It'd be good to have a review from a domain expert.

I am going to be out for the next 4 weeks, and will likely have little time reading patches. The patch looks good to land when a domain expert thinks it's fine.

Thanks for the reviews. I'll wait till @peter.smith or someone else who knows the domain has a look before landing.

amilendra marked 4 inline comments as done.Mar 10 2023, 2:58 AM
amilendra updated this revision to Diff 504077.Mar 10 2023, 3:03 AM

Add missing full stop.

amilendra marked an inline comment as done.Mar 10 2023, 3:03 AM

I've not finished checking yet, but thought it worth sending the comments that I have so far. Only thing that I can spot that looks wrong is that CMSE is M-profile only so the attributes check looks like it needs adjusting. The remainder are nit level comments.

lld/ELF/Arch/ARM.cpp
950

I know the language is taken from InputFiles.cpp, but I think it would be worth correcting here. I suggest
symbols is a parall array to the corresponding Elf symbol table.

1029

The function is called process, but it is not clear without reading the code what processing is done. I think it is doing two things:

  • Record the [__acle_se_<sym>, <sym>] pair for later processing.
  • Resolve any undefined symbols against the definitions in the CMSE Import Lib.

Would be good to make that explicit in the comment.

1030

Should be plural processArmCmseSymbols() as this processes more than one symbol.

lld/ELF/Driver.cpp
2816

Should be plural processArmCmseSymbols()

lld/ELF/InputFiles.cpp
181

The check below is a bit too loose as it will also include the A profile CPU arch > v8_M_Base as they are in the same tag. I think you'll need (pseudo code) && CPU_arch_profile = M

lld/ELF/SyntheticSections.h
1183

Is impLibMinAddr used? I can see it being assigned to, but can't see it being used for anything.

lld/test/ELF/arm-cmse-diagnostics.s
37

Thumbv9a doesn't have CMSE, only M-profile has CMSE.

A few more comments on some things that don't look right compared to the GNU ld interface.

lld/ELF/Arch/ARM.cpp
1068

I don't think this is needed. The non-secure app doesn't use --cmse-implib or --in-implib it just uses the raw object file produced by --out-implib.

If we do end up matching an undefined in the importlib from the secure application then something has gone badly wrong.

lld/test/ELF/arm-cmse-secure.s
13

The non-secure app doesn't use --in-implib and --cmse-implib it just uses implib.lib which is just a standard relocatable object. It doesn't actually need to know whether the definitions in the implib are CMSE related or not.

26

What is the destination of this bl? it should be bl __acle_se_entry the secure state should not use the import library. The import library is for the use of non-secure state.

Will be useful to add a test case where we take the address of _entry which should be crystallised as __acle_entry.

I think that's all the comments I have for this round.

lld/ELF/Arch/ARM.cpp
972

What about the binding? IIUC it is possible to have a weak definition.

Do we need to record the type? Or are these implicitly STT_FUNC so it doesn't matter?

1038

One thing that I don't think this will work for is wrapped secure entry functions. For example in secure state if I have a secure entry function foo, __acle_foo and I make another secure entry function __wrap_foo, acle_wrap_foo` then we'll only want to create one SG veneer for foo which calls __acle_foo where __acle_foo is the renamed __acle___wrap_foo. The original foo, __acle_foo are renamed to foo, __acle_foo are renamed to __real_foo and __acle___real_foo (in theory __acle___real_foo is useless is the wrapped entry function is no longer an entry function.

I think this will need quite a bit of faff to work properly. My suggestion is to search for __acle___wrap and error that wrapping secure entry functions is not supported. Could be worth adding support in a follow up patch.

Another thing we could check for is an attempt to wrap a secure entry function with a function that isn't a secure entry point. I.e. we have foo, __acle_foo but we only have __wrap_foo and no __acle__wrap_foo.

1094

I don't understand why we are sorting cmseSymVector by name of the non acle prefixed symbol. We partitions and sort the sgSections later on so I don't think the order of cmseSymVector matters? I could be missing something though.

1102

In the GNU ld implementation it is a warning when:

  • The in_implib contains an entry for foo but there is no __acle_foo defined.
  • When there is an in_implib but no out_implib when there is a new entry.

In both case this is an interface change so I do think it is worth warning about.

amilendra updated this revision to Diff 506604.Mar 20 2023, 8:29 AM

Address some of Peter's comments.
Expect how relocation against entry symbols are handled will be particularly contentious.
Couple of diagnostics suggested needs implementation and how the binding is handled needs investigation.

Thanks for the updates. I think we're getting close now. I've made an alternative suggestion to how we can redirect symbols which might be more easily localised to the ARM.cpp file.

lld/ELF/Arch/ARM.cpp
1089

Recommend something like:

"entry function '" + sym->getName() +"' from CMSE import library is not present in secure application"
lld/ELF/Relocations.cpp
1346 ↗(On Diff #506604)

While I think this will work. I'm thinking that there could be a less intrusive way of achieving this. As I understand it when we create SG veneers we have input sections with sym, and __acle_sym defined to the same address. All relocations and references are to sym.

In the current implementation, we redirect relocations from sym to __acle_sym and then it looks like we overwrite sym in finalize with the value of the SG Veneer.

Could we use a similar mechanism to SymbolTable::wrap() and essentially exchange __acle_sym with sym so that all references to sym go to __acle_sym we are then free to overwrite sym?

lld/ELF/SymbolTable.h
70

Can we use a struct with named values here rather than a pair for example:

struct EntryFunction {
  Symbol *acleSeSym;
  Symbol *sym;
};

I find nested pairs such as second.first and second.second hard to read.

amilendra updated this revision to Diff 517098.Apr 26 2023, 1:56 AM

Change warning message.

After rebasing, the inCMSEOutImpLib flag now results in a "SymbolUnion too large" assert,
so use a hash table to keep track of symbols in the output import library.

amilendra marked an inline comment as done.Apr 26 2023, 1:57 AM
amilendra updated this revision to Diff 523348.May 18 2023, 5:14 AM
  • Change implementation for redirecting references to secure gateways in secure apps to the corresponding entry functions.
  • Add diagnostic when a new entry function is added to the specified input import library, but there is no output import library specified.
amilendra marked 4 inline comments as done.May 18 2023, 5:20 AM
amilendra marked 6 inline comments as done.May 18 2023, 5:57 AM
amilendra added inline comments.
lld/ELF/Arch/ARM.cpp
972

From construction, only weak or global symbols will endup in the symbol table, and the linker errors for non STT_FUNC types so it should not matter, but I've added code to record binding and type just in case it will be useful later.

amilendra marked 4 inline comments as done.May 18 2023, 6:04 AM
amilendra updated this revision to Diff 523356.May 18 2023, 6:43 AM

Test change only. NFC.
Remove --cmse-implib when linking the non-secure app.

amilendra marked an inline comment as done.May 18 2023, 6:47 AM
amilendra marked an inline comment as done.May 18 2023, 10:32 AM
amilendra added inline comments.
lld/ELF/Relocations.cpp
1346 ↗(On Diff #506604)

Exchanging __acle_sym with sym in SymbolTable->symVector doesn't seem to work because InputFiles->symbols[] will continue to point to sym which is what the Relocations are using. So I had to change InputFiles->symbols[] to point to __acle_sym. However this implementation seems better than my previous attemp because it could be localized to only the Arm backend.

amilendra marked 2 inline comments as done.May 18 2023, 10:33 AM
amilendra added inline comments.
lld/ELF/Arch/ARM.cpp
1038

I'll add support for this in a subsequent patch.

Just a couple of suggestions. I think there is a way of implementing the symbol changes without needing an ObjFile member function. I've also recommended adding a comment to describe the implementation as a whole.

lld/ELF/Arch/ARM.cpp
949–1457

For the benefit of our future selves I think it would be useful to put a summary comment about how the LLD CMSE code fits together.

The Arm Cortex-M Security Extensions (CMSE) splits a system into two parts, the non-secure and secure states with the secure state inaccessible from the non-secure state, apart from an area of memory in secure state called the secure gateway which is accessible from non-secure state. The secure gateway contains one or more entry points which must start with a landing pad instruction SG. Arm recommends that the secure gateway consists only of secure gateway veneers, which are made up of a SG instruction followed by a branch to the destination in secure state. Full details can be found in Arm v8-M Security Extensions Requirements on Development Tools.

The CMSE model of software development requires the non-secure and secure states to be developed as two separate programs. The non-secure developer is provided with an import library defining symbols describing the entry points in the secure gateway. No additional linker support is required for the non-secure state.

Development of the secure state requires linker support to manage the secure gateway veneers. The management consists of:

  • Creation of new secure gateway veneers based on symbol conventions.
  • Checking the address of existing secure gateway veneers.
  • Warning when existing secure gateway veneers removed.

The secure gateway veneers are created in an import library, which is just an ELF object with a symbol table. The import library is controlled by two command line options:
--in-implib (specify an input import library from a previous revision of the program).
--out-implib (specify an output import library to be created by the linker).

The input import library is used to manage consistency of the secure entry points. The output import library is for new and updated secure entry points.

The symbol convention that identifies secure entry functions is the prefix acle_se_ for a symbol called name the linker is expected to create a secure gateway veneer if symbols acles_se_name and name have the same address. After creating a secure gateway veneer the symbol name labels the secure gateway veneer and the __acle_se_name labels the function definition.

The LLD implementation:

  • Reads an existing import library with importCmseSymbols().
  • Determines which new secure gateway veneers to create and redirects calls within the secure state to the __acle_se_ prefixed symbol with processCmseSymbols().
  • Models the SG veneers as a synthetic section.
1007

wrapSymbols does something similar but uses file->getMutableSymbols() are we able to use this instead as it will mean that we don't need redirectCmseSymbols() in ObjFile.

The code in wrapSymbols is:

parallelForEach(symtab->objectFiles, [&](InputFile *file) {
   MutableArrayRef<Symbol *> syms = file->getMutableSymbols();
   for (size_t i = 0, e = syms.size(); i != e; ++i)
     if (Symbol *s = map.lookup(syms[i]))
       syms[i] = s;
 });
amilendra updated this revision to Diff 526670.May 30 2023, 9:11 AM
amilendra marked an inline comment as done.

Address Peter's review comments.

amilendra marked 2 inline comments as done.May 30 2023, 9:12 AM
amilendra updated this revision to Diff 528773.Jun 6 2023, 3:37 AM
  • Fix an issue where the sections containing entry functions got garbage collected.
  • Write the secure gateways using halfwords and write16 to make it big-endian compatible.

I was able to successfully link a trusted-firmware secure application example with this patch. I've only got one minor observation about symbol order. Hopefully easy to sort out. From there I should be able to approve.

lld/ELF/Arch/ARM.cpp
1269

I noticed that for an example CMSE application that I tried, the symbols in the LLD import library were in reverse order of address. While there is no requirement for the symbols to be in ascending order it did look a bit strange.

The code here looks like it will be determined by the order that the StringMap iterator gives. Maps often have no defined order.

I recommend putting into a vector and sorting by address so we get a known address order.

MaskRay added inline comments.Jun 7 2023, 11:24 AM
lld/ELF/Arch/ARM.cpp
1269

Seems that we can use a SmallMapVector<Key, Value, 0>

lld/ELF/MarkLive.cpp
233

for (auto [symName, _] : symtab.cmseSymMap)

lld/test/ELF/arm-cmse-diagnostics.s
97

Just needs one pattern

amilendra updated this revision to Diff 530472.Jun 12 2023, 5:24 AM
  • Use SmallMapVector for cmseSymMap
  • Sort symbols in the import library in ascending order of address
amilendra updated this revision to Diff 530480.Jun 12 2023, 5:46 AM

Test refactoring.

amilendra marked 3 inline comments as done.Jun 12 2023, 5:50 AM
amilendra marked an inline comment as done.
peter.smith accepted this revision.Jun 12 2023, 5:57 AM

LGTM. Thanks for the patience with all the comments. Will be worth waiting a day or so to offer MaskRay a chance for any final comments.

MaskRay added inline comments.Jun 12 2023, 8:17 PM
lld/ELF/Arch/ARM.cpp
1114

Misuse of ;?

1141

auto [_, sym] avoids Defined *sym = p.second;

ditto below

1161
1171

Use count instead of find(...) ==/!= end()

1173

++newEntries

1183
1220

ArmCmseSGVeneer *. ditto everywhere

lld/test/ELF/arm-cmse-diagnostics.s
143

These comments are indented while the majority of comments don't. Consider removing indentation.

amilendra updated this revision to Diff 530841.Jun 13 2023, 2:59 AM

Apply the suggested refactoring.

amilendra marked 8 inline comments as done.Jun 13 2023, 2:59 AM

@MaskRay Do you have any further comments? Or is it okay to land this?

Because @MaskRay and @peter.smith have approved this patch previously, I will land it in a bit.
Hopefully, any further issues can be addressed incrementally as follow-up patches.
Thanks for the reviews/suggestions.

This revision was automatically updated to reflect the committed changes.

I am getting reports of build bot failures due to

/buildbot/llvm-project/lld/ELF/Arch/ARM.cpp:1300:29: error: expected primary-expression before ‘>’ token
 1300 |     osec->writeHeaderTo<ELFT>(++sHdrs);
      |                             ^
/buildbot/llvm-project/lld/ELF/Arch/ARM.cpp:1306:25: error: expected primary-expression before ‘>’ token
 1306 |       osec->writeTo<ELFT>(buf + osec->offset, tg);
      |                         ^

and warnings.

/home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/lld/ELF/Arch/ARM.cpp:1306:31: warning: left operand of comma operator has no effect [-Wunused-value]
/home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/lld/ELF/Arch/ARM.cpp:1306:31: warning: left operand of comma operator has no effect [-Wunused-value]
/home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/lld/ELF/Arch/ARM.cpp:1306:31: warning: left operand of comma operator has no effect [-Wunused-value]
/home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/lld/ELF/Arch/ARM.cpp:1306:31: warning: left operand of comma operator has no effect [-Wunused-value]

I am investigating a quick fix, but feel free to revert this if it blocks your work.

hctim added a subscriber: hctim.Jun 22 2023, 5:33 AM

Hi folks,

Looks like this broke the UBSan bulidbot. Unfortunately this patch was included in a set with a build-break, so it only showed up the build after and all the builds since.

The problem reproduces with a simple UBSan build, *as long as you use libcxx*. Using libstdc++ doesn't repro the problem.

You can reproduce the full buildbot using https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild, either buildbot_fast.sh or buildbot_bootstrap_ubsan.sh should repro it. Or, my faster variant for local repro was:

$ cmake \
-DLLVM_ENABLE_ASSERTIONS=ON \
-DCMAKE_C_COMPILER=$HOME/llvm-build/opt/bin/clang \
-DCMAKE_CXX_COMPILER=$HOME/llvm-build/opt/bin/clang++ \
-DLLVM_USE_LINKER=lld \
-GNinja \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_C_FLAGS="-fsanitize=undefined" \
-DCMAKE_CXX_FLAGS="-fsanitize=undefined" \
-DLLVM_ENABLE_PROJECTS="'clang;lld;libc'" \
-DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi" \
-DLLVM_LIBC_ENABLE_LINTING=OFF \
-DLLVM_USE_SANITIZER=Undefined \
-DLLVM_ENABLE_LIBCXX=ON `# NOTE: Super important!` \
/usr/local/google/home/mitchp/llvm/llvm
$ ninja check-lld

Not sure whether you need a modern libcxx. The buildbot does all the steps for you, but you can also have success doing a ninja install from a regular opt build, then adding libcxx to your linkerconfig. Something like sudo ldconfig /usr/local/lib/x86_64-unknown-linux-gnu/, you can find libcxx by find /usr/local/lib -name *libc++*.

Hi folks,

Looks like this broke the UBSan bulidbot. Unfortunately this patch was included in a set with a build-break, so it only showed up the build after and all the builds since.

The problem reproduces with a simple UBSan build, *as long as you use libcxx*. Using libstdc++ doesn't repro the problem.

You can reproduce the full buildbot using https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild, either buildbot_fast.sh or buildbot_bootstrap_ubsan.sh should repro it. Or, my faster variant for local repro was:

$ cmake \
-DLLVM_ENABLE_ASSERTIONS=ON \
-DCMAKE_C_COMPILER=$HOME/llvm-build/opt/bin/clang \
-DCMAKE_CXX_COMPILER=$HOME/llvm-build/opt/bin/clang++ \
-DLLVM_USE_LINKER=lld \
-GNinja \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_C_FLAGS="-fsanitize=undefined" \
-DCMAKE_CXX_FLAGS="-fsanitize=undefined" \
-DLLVM_ENABLE_PROJECTS="'clang;lld;libc'" \
-DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi" \
-DLLVM_LIBC_ENABLE_LINTING=OFF \
-DLLVM_USE_SANITIZER=Undefined \
-DLLVM_ENABLE_LIBCXX=ON `# NOTE: Super important!` \
/usr/local/google/home/mitchp/llvm/llvm
$ ninja check-lld

Not sure whether you need a modern libcxx. The buildbot does all the steps for you, but you can also have success doing a ninja install from a regular opt build, then adding libcxx to your linkerconfig. Something like sudo ldconfig /usr/local/lib/x86_64-unknown-linux-gnu/, you can find libcxx by find /usr/local/lib -name *libc++*.

Sorry for the inconvenience and thanks for the revert. I will investigate.

amilendra reopened this revision.Jul 3 2023, 4:07 AM

Reopening to fix the ASAN/UBSAN failures.

This revision is now accepted and ready to land.Jul 3 2023, 4:07 AM
amilendra updated this revision to Diff 536716.Jul 3 2023, 4:11 AM

Address Sanitizer Failure Fix

ArmCmseSGVeneer being a synthetic section resulted in SectionBase::getOutputSection() falling through to return cast<OutputSection>(this) and returning an unaligned pointer value when called by SectionBase::getVA(unsigned long) in ArmCmseSGVeneer::writeTo().

Fix this by modelling ArmCmseSGVeneer as an independent class with an offset within the ArmCmseSGSection (similar to Thunks), eliminating the need of the getVA() call.

FYI @MaskRay, @peter.smith or anyone else interested in this,
I've made some slight changes to fix the asan/ubsan errors.
Please let me know if there are objections in landing it like this.
(I think MaskRay is away till 5th of July, so I will wait till then)

Some suggestions for simplification.

lld/ELF/Arch/ARM.cpp
1335

IIUC at this point we have two partions:
P1
ordered by address existing implib veneers
P2
reverse ordered new veneers

I think you could simplify the code by using std::reverse(it, sgSections.end()); on the second partition rather than traversing it via llvm::reverse()

Then I think you would only need one much simpler loop.

for (size_t i; i < sgSections.size() ++i) {
  ArmCmseSGVeneer* s = sgSections[i];
  s->offset = i * s->size();
  Defined(file, StringRef(), s->sym->binding, s->sym->stOther, s->sym->type,
            s->offset | 1, s->size(), this)
        .overwrite(*s->sym);
}
lld/ELF/SyntheticSections.h
1151

Can the definition be moved into ARM.cpp? In ArmCMSESGVeneerSection the type is only used by name so we should be able to forward declare it here?

1156

If this is no longer a SyntheticSection you can make it a static const size_t as all SG veneers are the same size.

1181

As these are no longer sections perhaps worth calling them sgVeneers?

amilendra added inline comments.Jul 3 2023, 12:46 PM
lld/ELF/SyntheticSections.h
1151

I did try this, but it gave me an incomplete type error. It works for Thunk so can't see why it doesn't here. I'll give it another go.

amilendra updated this revision to Diff 536970.Jul 4 2023, 12:08 AM

Refactor using Peter's suggestions.

amilendra marked 4 inline comments as done.Jul 4 2023, 12:09 AM
amilendra added inline comments.
lld/ELF/Arch/ARM.cpp
1335

Actually the reverse is also no longer needed. Thanks for the suggestion.

amilendra marked an inline comment as done.Jul 4 2023, 12:10 AM
peter.smith accepted this revision.Jul 4 2023, 2:10 AM

Thanks for the updates, LGTM.

Because Peter has approved, and I don't think there has been significant changes since MaskRay previously approved, I will land this in a bit.
Please let me know if there are any objections.

This revision was automatically updated to reflect the committed changes.