[PATCH] D155808: [clang][driver] Missing the condition in IsARMBigEndain function.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added inline comments.



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:39
 // normalized triple so we must handle the flag here.
 bool arm::isARMBigEndian(const llvm::Triple , const ArgList ) {
+  if ((Triple.getArch() == llvm::Triple::arm ||

Is this the right place to fix?

I would expect it to be a precondition that the Triple was Arm or Thumb before 
calling isARMBigEndian?

For example
```
if (Triple.isARM() || Triple.isThumb() || Triple.isAArch64()) {
bool IsBigEndian = arm::isARMBigEndian(Triple, Args);
if (IsBigEndian)
  arm::appendBE8LinkFlag(Args, CmdArgs, Triple);
IsBigEndian = IsBigEndian || Arch == llvm::Triple::aarch64_be;
CmdArgs.push_back(IsBigEndian ? "-EB" : "-EL");
  }
```
Shouldn't this be refactored to only call isARMBigEndian for isARM and isThumb? 
Something like:
```
if ((Triple.isARM() || Triple.isThumb()) {
  bool BigEndian = arm::isARMBigEndian(Triple, Args);
  if (BigEndian)
arm::appendBE8LinkFlag(Args, CmdArgs, Triple);
  CmdArgs.push_back(IsBigEndian ? "-EB" : "-EL");
} else if (Triple.isAArch64)) {
  CmdArgs.push_back(Arch == llvm::Triple::aarch64_be ? "-EB" : "-EL");
}
```
This is a bit longer but it is easier to read and keeps ARM and AArch64 
separate.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155808/new/

https://reviews.llvm.org/D155808

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153430: [Clang][Driver] Warn on invalid Arm or AArch64 baremetal target triple

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

Thanks for the update! LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153430/new/

https://reviews.llvm.org/D153430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153430: [Clang][Driver] Warn on invalid Arm or AArch64 baremetal target triple

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

> clang: warning: mismatch between architecture and environment in target 
> triple 'aarch64-none-eabi'; did you mean 'aarch64-none-elf'? 
> [-Winvalid-command-line-argument]

That looks good to me. Would be happy with that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153430/new/

https://reviews.llvm.org/D153430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153430: [Clang][Driver] Warn on invalid Arm or AArch64 baremetal target triple

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

My initial reactions to seeing Invalid target triple aarch64-none-eabi; try 
aarch64-none-elf [*] were:

- Why is it invalid?
- I assumed it was an error message, and was about to write a comment until I 
saw it was a warning.

[X] now did you mean (thanks for making that change).

An alternative suggestion:

   selects the fallback GCC toolchain driver; did you mean 


This isn't something I'm going to die on a hill for. Perhaps add some more 
reviewers to get some opinions, I'm happy to go with the consensus.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153430/new/

https://reviews.llvm.org/D153430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153430: Warn on invalid Arm or AArch64 baremetal target triple

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I can confirm that we have seen several users of the LLVM Embedded Toolchain 
for Arm that work on both AArch64 and Arm make this mistake as it is easy to 
just alter an example triple by substituting the first element.

Given that this is a warning, invalid is perhaps a bit strong.

Perhaps "Unrecognized  for , did you mean 
--"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153430/new/

https://reviews.llvm.org/D153430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151308: -fsanitize=function: fix alignment fault on Arm targets.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In D151308#4369828 , @MaskRay wrote:

> In D151308#4367704 , @peter.smith 
> wrote:
>
>> This looks good to me. Will be worth waiting for a day to give the US time 
>> zone time to leave any comments.
>
> Thanks!
>
>> I note that this is also broken in -fsanitize=kcfi [*] 
>> (https://reviews.llvm.org/D135411) although fixing that is a separate patch. 
>> Would you be able to raise a github issue to cover that?
>
> `-fsanitize=kcfi` only supports aarch64 and x86-64 now. riscv64 is on the 
> plan.
>
>   % fclang -fsanitize=kcfi --traget=armv7-linux-gnueabi -c a.c
>   clang: error: unsupported option '--traget=armv7-linux-gnueabi'

IIUC initially kcfi was x86_64 and AArch64 only. In D135411 
 "generic" support was added for all targets, 
quoting from the description.

  The KCFI sanitizer emits "kcfi" operand bundles to indirect
  call instructions, which the LLVM back-end lowers into an
  architecture-specific type check with a known machine instruction
  sequence. Currently, KCFI operand bundle lowering is supported only
  on 64-bit X86 and AArch64 architectures.
  
  As a lightweight forward-edge CFI implementation that doesn't
  require LTO is also useful for non-Linux low-level targets on
  other machine architectures, add a generic KCFI operand bundle
  lowering pass that's only used when back-end lowering support is not
  available and allows -fsanitize=kcfi to be enabled in Clang on all
  architectures.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151308/new/

https://reviews.llvm.org/D151308

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151308: -fsanitize=function: fix alignment fault on Arm targets.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

This looks good to me. Will be worth waiting for a day to give the US time zone 
time to leave any comments.

I note that this is also broken in -fsanitize=kcfi [*] 
(https://reviews.llvm.org/D135411) although fixing that is a separate patch. 
Would you be able to raise a github issue to cover that?

As an end-to-end example for:

  typedef int Fptr(void);
  
  // pf could be Arm (bit 0 clear) or Thumb (bit 0 set)
  int f(Fptr* pf) {
return pf();
  }

This generates:

  f:
  .fnstart
  @ %bb.0:@ %entry
  push{r4, lr}
  mov r3, r0
  bic r0, r0, #1
  movwr2, #51966
  ldr r1, [r0, #-8]
  movtr2, #49413
  cmp r1, r2
  bne .LBB0_2
  @ %bb.1:@ %typecheck
  ldr r0, [r0, #-4]
  movwr1, #50598
  movtr1, #14001
  cmp r0, r1
  bne .LBB0_3
  .LBB0_2:@ %cont1
  pop.w   {r4, lr}
  bx  r3

Which gets the address of the signature and type correct, while preserving the 
thumb bit on the register used for the indirect branch.

-fsanitize=kcfi output is not correct for a Thumb destination:

  f:
  .fnstart
  // r0 will have thumb bit set if destination thumb
  ldr r1, [r0, #-4]
  movwr2, #50598
  movtr2, #14001
  cmp r1, r2
  bne .LBB0_2
  bx  r0
  .LBB0_2:
  .inst   0xe7ffdefe


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151308/new/

https://reviews.llvm.org/D151308

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148665: Change -fsanitize=function to place two words before the function entry

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

Apologies for the delay LGTM. I think there is a case for setting up the 
signature to be target specific, but that could in theory be done on demand 
when a target adds a clashing instruction.




Comment at: clang/lib/CodeGen/TargetInfo.h:205
   getUBSanFunctionSignature(CodeGen::CodeGenModule ) const {
-return nullptr;
+return llvm::ConstantInt::get(CGM.Int32Ty, 0xc105cafe);
   }

Is it worth making this target specific? Defaulting to 0xc105cafe for all 
targets, if that gets allocated in the future on any one target we can change 
it without having to test against all other targets.

For example on AArch64 this is is in the decoding space of SME instructions op0 
= 0b1 op1 = 0b. This may get allocated in the future. Although admittedly 
it is likely to be very rare to find a use of a SME instruction at the end of a 
function to cause it to get misidentified.

I suspect most targets have an explicit undefined instruction, such as UDF in 
AArch64 which is 0x where ? can be any value. On Arm there two separate 
4-byte encodings for Arm, Thumb of UDF.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148665/new/

https://reviews.llvm.org/D148665

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148785: -fsanitize=function: use type hashes instead of RTTI objects

[Peter Smith via Phabricator via cfe-commits]

peter.smith added inline comments.



Comment at: clang/test/CodeGenCXX/catch-undef-behavior.cpp:408
 
   // RTTI pointer check
+  // CHECK: [[CalleeTypeHashPtr:%.+]] = getelementptr <{ i32, i32 }>, <{ i32, 
i32 }>* [[PTR]], i32 -1, i32 1

MaskRay wrote:
> peter.smith wrote:
> > CalleeTypeHash check?
> `CalleeTypeHashPtr` seems clear. Do you mean to change `HashCmp` below to 
> `CalleeTypeHashMatch`?
> 
> 
Apologies; I meant the comment is stale, it still says RTTI pointer check



Comment at: llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll:92
 ; CHECK-NEXT: .Ltmp{{.*}}:
 ; CHECK-NEXT:   nop
 ; CHECK-NEXT:   .word   3238382334  // 0xc105cafe

MaskRay wrote:
> MaskRay wrote:
> > peter.smith wrote:
> > > samitolvanen wrote:
> > > > peter.smith wrote:
> > > > > Assuming the test is the equivalent of 
> > > > > `-fpatchable-function-entry=1,1` I think this is the wrong place for 
> > > > > the nop, I think it needs to be after the signature and the loads 
> > > > > adjusted. For example with -fsanitize=kcfi 
> > > > > -fpatchable-function-entries=1,1
> > > > > ```
> > > > > typedef int Fptr(void);
> > > > > 
> > > > > int function(void) {
> > > > >   return 0;
> > > > > }
> > > > > 
> > > > > int call(Fptr* fp) {
> > > > >   return fp();
> > > > > }
> > > > > ```
> > > > > Results in code like:
> > > > > ```
> > > > > .word   1670944012  // @call
> > > > > // 0x6398950c
> > > > > .Ltmp1:
> > > > > nop
> > > > > call:
> > > > > .Lfunc_begin1:
> > > > > .cfi_startproc
> > > > > // %bb.0:   // %entry
> > > > > ldurw16, [x0, #-8]
> > > > > movkw17, #50598
> > > > > movkw17, #14001, lsl #16
> > > > > cmp w16, w17
> > > > > b.eq.Ltmp2
> > > > > brk #0x8220
> > > > > .Ltmp2:
> > > > > br  x0
> > > > > .Lfunc_end1:
> > > > > ```
> > > > > Note the NOP is after the signature, with the `ldur` having an offset 
> > > > > of -8 and not the usual -4. I think you would need to make sure the 
> > > > > signature is a branch instruction for each target for this scheme to 
> > > > > work.
> > > > No, this looks correct to me. Note that in AsmPrinter the type hash is 
> > > > emitted after the patchable-function-prefix nops, while the KCFI type 
> > > > hash is emitted before them.
> > > My concern is more along the lines of how would this function be patched 
> > > if the code doing the patching were unaware of the signature. I'm not 
> > > familiar with how the kernel uses the nops so it could be that this is 
> > > won't be a problem in practice.
> > > 
> > > With -fsanitize=kcfi it looks like there is no difference to the code 
> > > doing the patching as the nops are in the same place with respect to the 
> > > function entry point and there is no fall through into the signature.
> > > 
> > > With -fsanitize=function anything patching the first nop as an 
> > > instruction would need to branch over the signature (unless the first 
> > > entry of the signature was a branch instruction, but that would mean a 
> > > target specific signature), obviously if the nop is patched with data and 
> > > isn't the entry point then this doesn't matter. The code doing the 
> > > patching would also need to know how to locate the nop ahead of the 
> > > signature.
> > The responsibility is on the user side to ensure that when M>0, they code 
> > patches one of the M NOPs to a branch over the signature (0xc105cafe). 
> > 
> > ```
> > // -fsanitize=function -fpatchable-function-entry=3,3
> > .Ltmp0:
> > nop
> > nop
> > nop# ensure there is a branch over the signature
> > .long   3238382334  # 0xc105cafe
> > .long   2772461324  # 0xa540670c
> > foo:
> > ```
> > 
> > In addition, `-fpatchable-function-entry=N,M` where M>0 is uncommon. 
> > `-fpatchable-function-entry=` didn't work with `-fsanitize=function` before 
> > my changes.
> > 
> > My concern is more along the lines of how would this function be patched if 
> > the code doing the patching were unaware of the signature. I'm not familiar 
> > with how the kernel uses the nops so it could be that this is won't be a 
> > problem in practice.
> 
> I think the patching code must be aware if the user decides to use 
> `-fpatchable-function-entry=N,M` where `M>0`, otherwise it's the pilot error 
> to use the option with `-fsanitize=function`.
> 
> `-fsanitize=function` is designed to be compatible with uninstrumented 
> functions (used as indirect call callees) and compatible with object files,  
> `-fpatchable-function-entry=N,M` functions, and regular functions. The call 
> site must use the fixed offset unaffected by `-fpatchable-function-entry=N,M`.
I don't have any objections here. Just wanted to make sure that we weren't 
breaking any 

[PATCH] D148785: -fsanitize=function: use type hashes instead of RTTI objects

[Peter Smith via Phabricator via cfe-commits]

peter.smith added inline comments.



Comment at: llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll:92
 ; CHECK-NEXT: .Ltmp{{.*}}:
 ; CHECK-NEXT:   nop
 ; CHECK-NEXT:   .word   3238382334  // 0xc105cafe

samitolvanen wrote:
> peter.smith wrote:
> > Assuming the test is the equivalent of `-fpatchable-function-entry=1,1` I 
> > think this is the wrong place for the nop, I think it needs to be after the 
> > signature and the loads adjusted. For example with -fsanitize=kcfi 
> > -fpatchable-function-entries=1,1
> > ```
> > typedef int Fptr(void);
> > 
> > int function(void) {
> >   return 0;
> > }
> > 
> > int call(Fptr* fp) {
> >   return fp();
> > }
> > ```
> > Results in code like:
> > ```
> > .word   1670944012  // @call
> > // 0x6398950c
> > .Ltmp1:
> > nop
> > call:
> > .Lfunc_begin1:
> > .cfi_startproc
> > // %bb.0:   // %entry
> > ldurw16, [x0, #-8]
> > movkw17, #50598
> > movkw17, #14001, lsl #16
> > cmp w16, w17
> > b.eq.Ltmp2
> > brk #0x8220
> > .Ltmp2:
> > br  x0
> > .Lfunc_end1:
> > ```
> > Note the NOP is after the signature, with the `ldur` having an offset of -8 
> > and not the usual -4. I think you would need to make sure the signature is 
> > a branch instruction for each target for this scheme to work.
> No, this looks correct to me. Note that in AsmPrinter the type hash is 
> emitted after the patchable-function-prefix nops, while the KCFI type hash is 
> emitted before them.
My concern is more along the lines of how would this function be patched if the 
code doing the patching were unaware of the signature. I'm not familiar with 
how the kernel uses the nops so it could be that this is won't be a problem in 
practice.

With -fsanitize=kcfi it looks like there is no difference to the code doing the 
patching as the nops are in the same place with respect to the function entry 
point and there is no fall through into the signature.

With -fsanitize=function anything patching the first nop as an instruction 
would need to branch over the signature (unless the first entry of the 
signature was a branch instruction, but that would mean a target specific 
signature), obviously if the nop is patched with data and isn't the entry point 
then this doesn't matter. The code doing the patching would also need to know 
how to locate the nop ahead of the signature.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148785/new/

https://reviews.llvm.org/D148785

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148827: -fsanitize=function: support C

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I agree with the reasoning. Can you update the documentation in 
clang/docs/UndefinedBehaviorSanitizer.rst to include C and state the known 
limitation. After D148573  it says:

  -  ``-fsanitize=function``: Indirect call of a function through a
   function pointer of the wrong type (C++ only).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148827/new/

https://reviews.llvm.org/D148827

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148785: -fsanitize=function: use type hashes instead of RTTI objects

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Should `HANDLER(__ubsan_handle_function_type_mismatch,"function")` be added to 
ubsan_minimal_runtime if this is supported in the minimal runtime?




Comment at: clang/lib/CodeGen/CGExpr.cpp:5382
+  getPointerAlign());
   llvm::Value *CalleeRTTIMatch =
+  Builder.CreateICmpEQ(CalleeTypeHash, TypeHash);

Would CalleeTypeHashMatch be a better name?



Comment at: clang/lib/CodeGen/CodeGenFunction.h:120
   SANITIZER_CHECK(FloatCastOverflow, float_cast_overflow, 0)   
\
-  SANITIZER_CHECK(FunctionTypeMismatch, function_type_mismatch, 1) 
\
+  SANITIZER_CHECK(FunctionTypeMismatch, function_type_mismatch, 0) 
\
   SANITIZER_CHECK(ImplicitConversion, implicit_conversion, 0)  
\

Presumably the signature is different to the original v0 shouldn't it be 2; or 
is it effectively so long since the last one that we can reuse the original 
without fear?



Comment at: clang/test/CodeGenCXX/catch-undef-behavior.cpp:408
 
   // RTTI pointer check
+  // CHECK: [[CalleeTypeHashPtr:%.+]] = getelementptr <{ i32, i32 }>, <{ i32, 
i32 }>* [[PTR]], i32 -1, i32 1

CalleeTypeHash check?



Comment at: llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll:92
 ; CHECK-NEXT: .Ltmp{{.*}}:
 ; CHECK-NEXT:   nop
 ; CHECK-NEXT:   .word   3238382334  // 0xc105cafe

Assuming the test is the equivalent of `-fpatchable-function-entry=1,1` I think 
this is the wrong place for the nop, I think it needs to be after the signature 
and the loads adjusted. For example with -fsanitize=kcfi 
-fpatchable-function-entries=1,1
```
typedef int Fptr(void);

int function(void) {
  return 0;
}

int call(Fptr* fp) {
  return fp();
}
```
Results in code like:
```
.word   1670944012  // @call
// 0x6398950c
.Ltmp1:
nop
call:
.Lfunc_begin1:
.cfi_startproc
// %bb.0:   // %entry
ldurw16, [x0, #-8]
movkw17, #50598
movkw17, #14001, lsl #16
cmp w16, w17
b.eq.Ltmp2
brk #0x8220
.Ltmp2:
br  x0
.Lfunc_end1:
```
Note the NOP is after the signature, with the `ldur` having an offset of -8 and 
not the usual -4. I think you would need to make sure the signature is a branch 
instruction for each target for this scheme to work.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148785/new/

https://reviews.llvm.org/D148785

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149946: [LoongArch] Define `ual` feature and override `allowsMisalignedMemoryAccesses`

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

No objections for moving out m_arm_Features_Group or adding the alias. It looks 
like that is currently unused i.e. no driver filters out the 
m_arm_Features_Group. I can't comment on the LoongArch specific parts of the 
patch.

I think you may want to note in the help that the option is not universally 
supported. Perhaps add a test to show that it remains unused on a target that 
does not support it. For example for an x86_64 target:

  clang: warning: argument unused during compilation: '-munaligned-access' 
[-Wunused-command-line-argument]




Comment at: clang/include/clang/Driver/Options.td:3656
-def munaligned_access : Flag<["-"], "munaligned-access">, 
Group,
-  HelpText<"Allow memory accesses to be unaligned (AArch32/AArch64 only)">;
-def mno_unaligned_access : Flag<["-"], "mno-unaligned-access">, 
Group,

I think this is AArch32/AArch64/LoongArch only. I don't think this patch adds 
support for all other Targets.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149946/new/

https://reviews.llvm.org/D149946

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148665: Change -fsanitize=function to place two words before the function entry

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

My apologies for not responding. If I've got this right there are 4 related 
patches:
D148573  (approved)
D148785  Use type hashes rather than RTTI 
D148827  support C
D148665  (this one)

My initial impressions is that this makes -fsanitize=function look more like 
-fsanitize=kcfi which makes it accessible from C and available to more targets. 
Is there anything that we lose in the process of making these changes? For 
example I would expect RTTI to have more information available than a type 
hash, although this might not make any functional difference.

I'll try and look over the next few days and leave some comments, apologies a 
bit busy at work at the moment so I can't promise anything speedy.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148665/new/

https://reviews.llvm.org/D148665

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149458: [Driver] Pass --target2= to linker from baremetal toolchain

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

LGTM. This is consistent with the target2 support that I added to LLD several 
years ago in https://reviews.llvm.org/D25684 will be good to give some time for 
other reviewers to add any comments/objections before committing.

Other references:

- aaelf32 
https://github.com/ARM-software/abi-aa/blob/main/aaelf32/aaelf32.rst#5617static-miscellaneous-relocations
- ehabi32 
https://github.com/ARM-software/abi-aa/blob/main/ehabi32/ehabi32.rst#542relocations

The latter states that ABS is the right value for bare-metal, but I think it 
ended up being implemented as REL for the GNU bare-metal personality routines.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149458/new/

https://reviews.llvm.org/D149458

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148573: Port -fsanitize=function to AArch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

With moving the signature before the function entry this looks good to me. I'm 
not so familiar with the code in https://reviews.llvm.org/D148665 would ideally 
find someone a bit more experienced to take a look at that part. If I'm the 
only one available I can take a look but may take a bit more time.

We're looking to clarify when a BTI is required in the ABI 
https://github.com/ARM-software/abi-aa/issues/196 starting with some design 
options.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148573/new/

https://reviews.llvm.org/D148573

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148573: Port -fsanitize=function to AArch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

One other small thing. The docs page 
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html would need the 
supported architectures updating:

  -fsanitize=function: Indirect call of a function through a function pointer 
of the wrong type (Darwin/Linux, C++ and x86/x86_64 only).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148573/new/

https://reviews.llvm.org/D148573

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148573: Port -fsanitize=function to AArch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

As it stands I think this may have problems with -mbranch-protection. In that 
case we'll need a `BTI c` to be the target of the indirect branch. I'm guessing 
something like:

  _Z3funv
  BTI C ; In hint space
  B . + 8 
  .word .L__llvm_rtti_proxy-_Z3funv

Otherwise when the indirect call is made then it will fail on a system with BTI 
enabled.

Not too sure how much of a problem this is for the implementation. The `BTI c` 
can't be used as a signature, I guess the code in the caller could check the 
value at `_z3funv + 4` . The feature could be marked as incompatible with 
`-mbranch-protection`. I guess it may not work well with patchable functions 
either.

I would expect the emitGlobalConstant to emit data. This would be visible in 
the object file as we'd have:

  $d
  
  
  $x
  instructions.

At the moment I don't think that this would affect anything except 
disassemblers, and the LLD cortex-a53 eratta work around which excludes $d from 
the disassembly. It is something that it could be worth fixing, expecially if 
there is a `BTI C` involved.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148573/new/

https://reviews.llvm.org/D148573

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146757: [Driver] Enable defining multilib print options independently of flags

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Approach looks good to me. Some suggestions, mostly around documenting the 
interface.




Comment at: clang/include/clang/Driver/Multilib.h:56
   /// This is enforced with an assert in the constructor.
   Multilib(StringRef GCCSuffix = {}, StringRef OSSuffix = {},
+   StringRef IncludeSuffix = {}, const flags_list  = 
flags_list(),

I think it is worth adding some information from the commit message to the 
comment. In particular what the default behaviour is and what, if any, 
restrictions are there on the PrintOptionsList.

For example there is a comment in the  assert in the constructor body that 
probably ought to be in the header
```
// If PrintOptions is something other than List then PrintOptionsList must be
// empty.
```

Are there any restrictions on the format of PrintOptionsList for it to print 
correctly? For example does each option need to be prefixed with `-`? 



Comment at: clang/include/clang/Driver/Multilib.h:58
+   StringRef IncludeSuffix = {}, const flags_list  = 
flags_list(),
+   PrintOptionsType PrintOptions = PrintOptionsType::PlusFlags,
+   const flags_list  = flags_list());

How would someone using `makeMultilib()` from `MultilibBuilder` use these 
parameters? If they can't do they need to? If so we may need something like a 
makeMultilib overload to supply the extra parameters.



Comment at: clang/lib/Driver/Multilib.cpp:44
+  assert(PrintOptions == PrintOptionsType::List || PrintOptionsList.empty());
+}
+

If there are any restrictions on the strings in PrintOptionsList could be worth 
assertions. Please ignore if there are no restrictions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146757/new/

https://reviews.llvm.org/D146757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143587: [Docs] Multilib design

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

I've set approved from the Arm side. Please leave some time for people in the 
US time zone to leave any final comments or ask for extensions. I've left a 
suggestion that can be applied prior to commit if you want to take it up.




Comment at: clang/docs/Multilib.rst:56
+them, and the criteria by which they are selected.
+
+Multilib processing

Can we add a forward reference to the Stable Interface design principle, or 
perhaps move it here so that it is more obvious?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143587/new/

https://reviews.llvm.org/D143587

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143075: BareMetal ToolChain multilib layering

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

I've set approved from the Arm side. Please leave some time for people in the 
US time zone to leave any final comments or ask for extensions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143075/new/

https://reviews.llvm.org/D143075

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143059: [Driver] Enable selecting multiple multilibs

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

I've set approved from the Arm side. Please leave some time for people in the 
US time zone to leave any final comments or ask for extensions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143059/new/

https://reviews.llvm.org/D143059

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142986: Enable multilib.yaml in the BareMetal ToolChain

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

I've set approved from the Arm side. Please leave some time for people in the 
US time zone to leave any final comments or ask for extensions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142986/new/

https://reviews.llvm.org/D142986

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142933: Add -print-multi-selection-tags-experimental option

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

I've set approved from the Arm side. Please leave some time for people in the 
US time zone to leave any final comments or ask for extensions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142933/new/

https://reviews.llvm.org/D142933

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142932: Multilib YAML parsing

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I've set approved from the Arm side. Please leave some time for people in the 
US time zone to leave any final comments or ask for extensions. I've left some 
comments that can be applied prior to commit if you want to take them up.




Comment at: clang/include/clang/Driver/Multilib.h:66
+  /// options and look similar to them, and others can be defined by a
+  /// particular multilib.yaml. A multilib is considered compatible if its tags
+  /// are a subset of the tags derived from the Clang command line options.

With the context of https://discourse.llvm.org/t/rfc-multilib/67494 and 
potential other formats. I think it will be worth making this comment not 
specific to multilib.yaml. For example if there is another non-YAML DSL that 
has tags.

For example `multilib DSL` rather than `multilib.yaml`  



Comment at: clang/include/clang/Driver/Multilib.h:140
+
+  bool parse(llvm::MemoryBufferRef, llvm::SourceMgr::DiagHandlerTy = nullptr,
+ void *DiagHandlerCtxt = nullptr);

With reference to https://discourse.llvm.org/t/rfc-multilib/67494 perhaps 
rename this to `parseMultilibYaml` as parse is generic, yet there is no 
indication it is working on a yaml file here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142932/new/

https://reviews.llvm.org/D142932

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145567: [Driver] Rename multilib flags to tags

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.

Actually click the button this time to set approval, see previous comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145567/new/

https://reviews.llvm.org/D145567

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145567: [Driver] Rename multilib flags to tags

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I've set approved from the Arm side. Please leave some time for people in the 
US time zone to leave any final comments or ask for extensions. 
From looking at the source code alone - assuming that most people would not 
track down the commit message/review for extra context - I found it difficult 
to work out the convention for when Flags is used and when Tags is used. I've 
made a suggestion for some comments. This can be applied prior to commit if you 
want to take it up.




Comment at: clang/lib/Driver/ToolChains/CommonArgs.h:202
+ Multilib::tag_set );
 
 void addX86AlignBranchArgs(const Driver , const llvm::opt::ArgList ,

I can see the reason to keep the name `addMultilibFlag`. At this point is the 
tag_set expected to be simplified tags or full command line flags. If it is the 
former I think it would be good to change Flags to Tags here.

May also be useul to add a \p for Flags (or Tags) if there are any 
requirements, or just useful information on what it is expected to be.

Parameter name also applies to CommonArgs.cpp below.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145567/new/

https://reviews.llvm.org/D145567

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142905: [Driver] Change multilib selection algorithm

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

I've set approved from the Arm side. Please leave some time for people in the 
US time zone to leave any final comments or ask for extensions. I've left a 
suggestion for a comment, but this can be applied prior to commit if you want 
to take it up.




Comment at: clang/include/clang/Driver/Multilib.h:67
+  /// a subset of the flags derived from the Clang command line options.
+  const flag_set () const { return Flags; }
 

Would be good to reference the Multilib Design document's definition and 
examples of (what will be renamed in D145567 to tags).

This may require moving the design doc down the stack, probably not necessary 
if everything is committed at once.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142905/new/

https://reviews.llvm.org/D142905

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143763: [Clang] Add clangMinimumVersion to multilib.yaml

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

No objections to MaskRay's suggestion to merge with an earlier patch. I've made 
some suggestions to make the error messages be a bit more specific about what 
is wrong.




Comment at: clang/lib/Driver/Multilib.cpp:185
+if (M.ClangMinimumVersion.empty())
+  return "missing required key 'clangMinimumVersion'";
+

Perhaps
"missing required key 'clangMinimumVersion' expect MAJOR.MINOR.PATCHLEVEL" to 
give an idea what the key is expecting?



Comment at: clang/lib/Driver/Multilib.cpp:195
+if (MinVerStrings.size() != 3)
+  return "not a valid version string \"" + M.ClangMinimumVersion + "\"";
+

Can we add why the version string isn't valid? For example "expected format 
MAJOR.MINOR.PATCHLEVEL got " + M.CLangMinimumVersion ...





Comment at: clang/lib/Driver/Multilib.cpp:200
+  if (S.getAsInteger(10, V))
+return "not a valid version string \"" + M.ClangMinimumVersion + "\"";
+  ClangMinimumVersion.push_back(V);

Similarly, can we say what we were expecting. For example all components must 
be decimal integers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143763/new/

https://reviews.llvm.org/D143763

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143075: BareMetal ToolChain multilib layering

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

No comments on the implementation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143075/new/

https://reviews.llvm.org/D143075

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143587: [Docs] Multilib design

[Peter Smith via Phabricator via cfe-commits]

peter.smith added inline comments.



Comment at: clang/docs/Multilib.rst:125
+``-fno-exceptions`` multilib variant need only contain C++ libraries.
+
+Stability

Although implicit in the mechanism, is it worth highlighting that layered 
multib.yaml authors will need to make sure that the includes and the libraries 
in the layer need to be complete enough to mask any incompatibilities? 

For example if in the `-fno-exceptions` case there exists a library `libX.a` 
that is affected by `-fno-exceptions` but is present in the "Core directory" 
but not the layered no-exceptions directory then the exceptions libX.a will be 
selected.

/fp/libX.a (should have a noexcept variant in noexcept but multilib implementer 
gets it wrong!)
/fp/libY.a
/fp/lib/noexecpt/libY.a

execeptions is perhaps not the best example here as including a file with 
exceptions when the intention is `-fno-exceptions` is not optimal, but it will 
work. If there is layering for floating point calling conventions then a mix 
could result in a broken program.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143587/new/

https://reviews.llvm.org/D143587

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142933: Add -print-multi-selection-flags argument

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Thanks for the updates, changes look good to me. I think that we can get away 
without trying to break the floating point options into flags, at least for how 
floating point units are available in hardware today.




Comment at: clang/lib/Driver/ToolChain.cpp:173
 }
 
+std::vector

michaelplatings wrote:
> peter.smith wrote:
> > IIUC this needs to contain the union of all command-line options used by 
> > all clients of the YAML but not any of the existing hard-coded Multilibs?
> > 
> > There is a possibility that this function could get large over time, with 
> > many Toolchain and Architecture specific flags? It is it worth delegating 
> > some of the work to the Toolchain, for example at the moment the BareMetal 
> > driver won't support the sanitizers so this could be processed by a 
> > Toolchain specific function (Fuchsia in this case?). There's likely to be a 
> > lot of options used by many Multilib implementations so there is still 
> > scope for putting stuff here. We would need to know what driver and 
> > architecture we are using here though.
> > 
> > If we had already constructed the MultilibSet it could be possible to 
> > filter the flags against what the MultilibSet actually accepts?
> > 
> > Is it worth making that clear, or have some way for the other non YAML 
> > users to call print_multi_selection_flags? Or is the option purely for 
> > developing/debugging multilib as I'd not expect the output of 
> > print_multi_section flags to be used by a user of clang. In that case a 
> > translation of what command-line flags influence multilib selection 
> > (ideally filtered) through the processed YAML file would be great.
> Correct, we don't yet need to cater for existing hard-coded Multilibs.
> 
> I did think about putting some argument processing in ToolChain subclasses 
> but for now I'm inclined to say YAGNI. If it's needed later on that's an easy 
> refactoring. With the current form, there's less likelihood of different 
> ToolChain subclasses diverging in the style of attributes they emit.
> 
> MultilibSet can use a regex, and both matching and not matching can be 
> significant, so I'm not sure it makes sense to filter against that.
> 
> `-print-multi-selection-flags-experimental` can be used completely 
> independently of any YAML or whether the current ToolChain class actually 
> supports multilib. `-print-multi-selection-flags-experimental` is intended 
> primarily for developing/debugging multilib.
Thinking about Arm floating point modelling (not really relevant to the 
mechanism here). Certainly since the Cortex cores the instructions available to 
an FPU are tied to the architecture. I.e. you cannot mix and match a FPU from 
an earlier or later architecture revision in practice, even if you can put it 
on the command-line in theory. I think this means that in the majority of cases 
we just need to match the floating point libraries with the architecture. The 
exceptions are the floating point variants with cut-down functionality:
* single-precision only (a nightmare of hard-float for floats and soft-float 
for doubles)
* d16 (only 16 double precision registers available rather than 32. I would 
expect this to be interface compatible with code compiled with 32-double 
precision registers as the calling convention doesn't change). All 
single-precision only are also d16.

I think these fpu options are restricted to Cortex-M and Cortex-R.

For a small number of targets we'd likely need at least two (single only, and 
d16) and at most 3 (single only, d16, and full) floating point variants. We'd 
need to map the fpu name suitable for the architecture but I don't think we 
would want to work too hard trying to map an arbitrary floating point unit that 
there is no available hardware for. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142933/new/

https://reviews.llvm.org/D142933

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143587: [Docs] Multilib design

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Thanks for the update, one nit in the language, otherwise looks good to me. It 
is a good reflection of the implementation.




Comment at: clang/docs/Multilib.rst:250
+
+The API need only multilib selection based on only a limited set of command
+line arguments. Later LLVM versions can add support for multilib selection from

"The API need only multilib selection" doesn't look right. Perhaps "The API can 
do multilib selection"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143587/new/

https://reviews.llvm.org/D143587

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143059: [NFC] Enable selecting multiple multilibs

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

A couple of small suggestions but otherwise looks good to me.




Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:233
+  std::string Result = computeBaseSysRoot(getDriver(), getTriple());
+  if (!SelectedMultilibs.empty()) {
+Result += SelectedMultilibs.back().osSuffix();

Nit: the braces can be omitted here. No strong opinion as it may be better to 
keep consistency with the rest of the file rather than LLVM as a whole.

https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements



Comment at: clang/lib/Driver/ToolChains/Fuchsia.cpp:319
+SelectedMultilibs.erase(SelectedMultilibs.begin(),
+SelectedMultilibs.end() - 1);
+

SelectedMultilibs.end() - 1 makes me a little nervous. This will work for the 
current container type (I think the standard requires it for vector and I don't 
think SmallVector would deviate from it. However in the unlikely event that the 
container changes and this isn't valid then this could break.

Perhaps use std::advance(SelectedMultilibs.end(), -1) which is more likely to 
break at compile time if this occurs.

Alternatively if this is just erasing all but the last element, maybe extract 
it, clear the container and reinsert.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143059/new/

https://reviews.llvm.org/D143059

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142986: Enable multilib.yaml in the BareMetal ToolChain

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Looks good to me. Some small suggestions around execute-only as I expect this 
to be a useful multilib variant.




Comment at: clang/lib/Driver/ToolChain.cpp:198
   unsigned FPUKind = llvm::ARM::FK_INVALID;
-  tools::arm::getARMTargetFeatures(D, Triple, Args, Features, false, );
+  tools::arm::getARMTargetFeatures(D, Triple, Args, Features, false, false,
+   );

Now that there are two boolean parameters, could you put a prefix comment to 
show which one is which for example:
/* ForAs */ false, /* GenExecuteOnly */ 



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:779
   // This only makes sense for the compiler, not for the assembler.
-  if (!ForAS) {
+  if (!ForAS && GetExecuteOnly) {
 // Supported only on ARMv6T2 and ARMv7 and above.

I can see execute-only being used in a multilib configuration as it can impact 
code-generation significantly yet is a requirement for a system aiming to have 
all code execute-only.

Rather than a specific `GenExecuteOnly` flag, would it be better for something 
like `ForMultilib`  like `ForAS` and then we could move it to not give the 
error message when doing `ForMultilib` the name would also generalise to other 
parts.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142986/new/

https://reviews.llvm.org/D142986

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142905: Change multilib selection algorithm

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Thanks for the updates, I'm happy with the changes and don't have any more 
comments. Happy to give my implicit approval.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142905/new/

https://reviews.llvm.org/D142905

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142893: [NFC] Class for building MultilibSet

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Thanks for the updates, I'm happy with the changes and don't have any more 
comments. Happy to give my implicit approval.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142893/new/

https://reviews.llvm.org/D142893

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143587: [Docs] Multilib design

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Thanks very much for writing this. Will be worth updating the RFC with a link 
as I think that this is an approachable place to comment on the format and 
selection model without the implementation detail. I'm happy with what has been 
written so far. My suggestions are mostly additions rather than modifications.

Could be worth adding a design principles section at the end, this may help 
someone looking back rationalise the choices made. For example I think that it 
is an important decision to expand the command-line options in clang prior to 
multi-lib selection to avoid another implementation of the complex architecture 
feature selection code in multilib.

May be worth a quick description of how we might model some awkward cases like 
position independent and non-position independent code. This is analagous to 
soft and softfp.

  Position independence is an awkward case to model, which may be worth showing 
as an example.
  * if my incoming flags include position independent code [PIC] then I need to 
select the [PIC] libraries, which implies that we want a [PIC] flag.
  * if my incoming flags do not include position independent code then I can 
use [PIC] libraries, but I would prefer to use not PIC if it were available.
  If we did have separate PIC and non PIC choices then I think this would need 
two features [PIC] and [non-PIC] with incoming non-position independent code 
having both [PIC, non-PIC] so it could match both, but position indendent code 
would only have [PIC]. Presumably we'd need the [non-PIC] variant sorted after.




Comment at: clang/docs/Multilib.rst:102
+There are two options permitted:
+#. Use only the *last* matching multilib variant.
+#. Use all matching variants, thereby layering them.

May be worth indicating that this is to support compatibility with existing 
clang multilib, we expect most implementations to use all matching variants.



Comment at: clang/docs/Multilib.rst:105
+
+This decision is hard-coded per ToolChain subclass.
+

Considering the BareMetal ToolChain is used by RISC-V, Arm and AArch64 is there 
scope for a different choice per toolchain. I guess that we could put a 
predicate function for the ToolChain that could change due to architecture.



Comment at: clang/docs/Multilib.rst:122
+``-fno-exceptions`` multilib variant need only contain C++ libraries.
+
+Stability

Although it can be worked out by looking at the example, which I think is your 
intention from the comment. It may be worth a section on Syntax that extracts 
some of these.
```
Syntax of multilib.yaml
===
```
Some suggestions/questions:
* Does the file have to contain both variants: and flagmap: are either of them 
optional?
* Do variants have to come before flagmap, or is order not important.
* Will be worth highlighting that the order of the variants may be significant 
depending on the toolchain.
* Each variant is made up of dir: (path added to sysroot?), flags: [comma 
separated list of flags] and printArgs: [comma separated list of command line 
options for -print-multi-lib and -print-multi-directory, but not important for 
multilib selection]
* Each flagmap is made up of a POSIX extended syntax regex: that matches a 
flag, and (one of?) matchFlags/noMatchFlags which either adds or removes 
respecively a [comma separated list of flags] 



Comment at: clang/docs/Multilib.rst:169
+# considered a match.
+flags: [V6]
+# If a user invokes clang with -print-multi-lib then the arguments it

Although very Arm specific. I'd use [V6M] here as ArmV6 (pre-cortex so no 
explicit profile) is very different to ArmV6-m. Also in regex section below.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143587/new/

https://reviews.llvm.org/D143587

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142933: Add -print-multi-selection-flags argument

[Peter Smith via Phabricator via cfe-commits]

peter.smith added inline comments.



Comment at: clang/include/clang/Driver/Options.td:4162
 def print_multi_lib : Flag<["-", "--"], "print-multi-lib">;
+def print_multi_selection_flags : Flag<["-", "--"], 
"print-multi-selection-flags-experimental">,
+  HelpText<"Print the flags used for selecting multilibs (experimental)">;

any particular reason for using multi rather than multilib? Is there any intent 
to use this anywhere else?



Comment at: clang/include/clang/Driver/ToolChain.h:289
+  /// Get flags suitable for multilib selection, based on the provided clang
+  /// command line arguments. The arguments aren't suitable to be used directly
+  /// for multilib selection because they are not normalized and normalization

Could be worth using "The command line arguments ..." when I first read I 
didn't pick up on the distinction between flags and arguments. IIUC this 
function translates command line arguments into normalised multilib selection 
flags.



Comment at: clang/include/clang/Driver/ToolChain.h:303
+  ///   returned separately. There may not be valid syntax to express this as a
+  ///   clang argument so an pseudo argument syntax must be used instead.
+  /// To allow users to find out what flags are returned, clang accepts a

typo "a pseudo"



Comment at: clang/lib/Driver/ToolChain.cpp:173
 }
 
+std::vector

IIUC this needs to contain the union of all command-line options used by all 
clients of the YAML but not any of the existing hard-coded Multilibs?

There is a possibility that this function could get large over time, with many 
Toolchain and Architecture specific flags? It is it worth delegating some of 
the work to the Toolchain, for example at the moment the BareMetal driver won't 
support the sanitizers so this could be processed by a Toolchain specific 
function (Fuchsia in this case?). There's likely to be a lot of options used by 
many Multilib implementations so there is still scope for putting stuff here. 
We would need to know what driver and architecture we are using here though.

If we had already constructed the MultilibSet it could be possible to filter 
the flags against what the MultilibSet actually accepts?

Is it worth making that clear, or have some way for the other non YAML users to 
call print_multi_selection_flags? Or is the option purely for 
developing/debugging multilib as I'd not expect the output of 
print_multi_section flags to be used by a user of clang. In that case a 
translation of what command-line flags influence multilib selection (ideally 
filtered) through the processed YAML file would be great.



Comment at: clang/lib/Driver/ToolChain.cpp:220
+
+  switch (Triple.getArch()) {
+  case llvm::Triple::arm:

Even though it might still live in this file, might be worth having all options 
for a particular arch in a separate function. For example:
```
case llvm::Triple::arm:
case llvm::Triple::armeb:
case llvm::Triple::thumb:
case llvm::Triple::thumbeb:
  addArmArchFlags(); // not put any thought into parameters etc. 
```



Comment at: clang/lib/Driver/ToolChain.cpp:225
+  case llvm::Triple::thumbeb: {
+std::vector Features;
+unsigned FPUKind = llvm::ARM::FK_INVALID;

Can we handle the A profile situation where effectively we have v8 + a list of 
features, with each v8.x and v9.y enabling a set of features 
https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html 

Ideally we would want to be able to do things like map v8.5-a+sve+sve2 and v9 
to the same libraries. 



Comment at: clang/lib/Driver/ToolChain.cpp:244
+#include "llvm/TargetParser/ARMTargetParser.def"
+default:
+  llvm_unreachable("Invalid FPUKind");

I'm wondering if instead of using the FPU name we use a string representation 
of the properties for the flags. For example FK_VFPV3_D16 this would be akin to 
expanding march=armv8.6 to a list of target features. We'd need to make a 
stable string representation but I think that should be possible.



Comment at: clang/test/Driver/print-multi-selection-flags.c:40
+// CHECK-MVE: target=thumbv8.1m.main-none-unknown-eabihf
+
+// RUN: %clang -print-multi-selection-flags-experimental 
--target=arm-none-eabi -march=armv8.1m.main+mve+nofp | FileCheck 
--check-prefix=CHECK-MVENOFP %s

Would be useful to have some Armv7-A and Armv8-A tests as well. In particular 
I'm thinking of Arm-v8 and Arm-v8.1 as LSE atomics appear in v8.1. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142933/new/

https://reviews.llvm.org/D142933

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142905: Change multilib selection algorithm

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

A couple of small stylisitic suggestions.




Comment at: clang/include/clang/Driver/Multilib.h:31
 public:
-  using flags_list = std::vector;
+  using flags_list = std::set;
+  using arg_list = std::vector;

would flags_set be a better name. I'm guessing we're using set as we want 
uniqueness and ordering? 

If we don't need ordering then we may be able to use 
https://llvm.org/docs/ProgrammersManual.html#llvm-adt-stringset-h



Comment at: clang/include/clang/Driver/Multilib.h:106
+  /// Select compatible variants
+  std::vector select(const Multilib::flags_list ) const;
+

Worth using multilib_list for consistency with the rest of the file?

Is this meant to be an overload for bool select below? I mention it as it has a 
different return type. Perhaps use selectCompatible? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142905/new/

https://reviews.llvm.org/D142905

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142893: [NFC] Class for building MultilibSet

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Looks good so far. Some stylistic suggestions.




Comment at: clang/include/clang/Driver/Multilib.h:25
 namespace driver {
 
 /// This corresponds to a single GCC Multilib, or a segment of one controlled

It took a bit of reading to work out what the relationship between Multilib, 
MultilibVariant, MultilibSet and MultilibVariantBuilder are and how they are 
expected to be used.

IIUC MultilibVariant and MultilibVariantBuilder are used to construct Multilib 
and MultilibSet, which are expected to be immutable post construction.

Will be worth either a comment describing the relation ship or potentially 
write up the design in the clang docs and reference it from here. 



Comment at: clang/include/clang/Driver/Multilib.h:40
 public:
   Multilib(StringRef GCCSuffix = {}, StringRef OSSuffix = {},
+   StringRef IncludeSuffix = {}, int Priority = 0,

Could be worth documenting the requirement for the strings to start with a `/` 
or be empty as the constructor implementation in a different file will assert 
if they aren't?



Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:32
 
-static Multilib makeMultilib(StringRef commonSuffix) {
-  return Multilib(commonSuffix, commonSuffix, commonSuffix);
+static MultilibBuilderVariant makeMultilib(StringRef commonSuffix) {
+  return MultilibBuilderVariant(commonSuffix, commonSuffix, commonSuffix);

Worth making this a lambda in findRISCVMultilibs? Purely subjective opinion 
here as there could be an attempt to limit changes.



Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:44
   if (TargetTriple.isRISCV64()) {
-Multilib Imac = 
makeMultilib("").flag("+march=rv64imac").flag("+mabi=lp64");
-Multilib Imafdc = makeMultilib("/rv64imafdc/lp64d")
-  .flag("+march=rv64imafdc")
-  .flag("+mabi=lp64d");
+auto Imac = makeMultilib("").flag("+march=rv64imac").flag("+mabi=lp64");
+auto Imafdc = makeMultilib("/rv64imafdc/lp64d")

Wondering if we've lost information here by going to auto. We're really making 
a MultilibVariant here. Perhaps worth renaming makeMultilib to 
makeMultilibVariant?



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1049
 
-static Multilib makeMultilib(StringRef commonSuffix) {
-  return Multilib(commonSuffix, commonSuffix, commonSuffix);
+static MultilibBuilderVariant makeMultilib(StringRef commonSuffix) {
+  return MultilibBuilderVariant(commonSuffix, commonSuffix, commonSuffix);

Similar comment to BareMetal, is it worth renaming to makeMultilibVariant



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1737
   });
 
   Multilib::flags_list Flags;

Although I'm not too bothered myself, some people prefer to keep changes in 
whitespace to a separate NFC patch for the benefit of git annotate.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142893/new/

https://reviews.llvm.org/D142893

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136385: Add support for MC/DC in LLVM Source-Based Code Coverage

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I've got about as far as the clang changes. As I mentioned in Discourse I'm not 
familiar enough in this area to give good feedback on the implementation, most 
if not all my comments fall under the bike shedding category. Will need to 
leave the high-level feedback to people that are more experienced in this area.

Thanks very much for uploading the design doc pdf, that was very useful. 
Although likely not for this patch as it is large enough already, will be worth 
updating https://llvm.org/docs/CoverageMappingFormat.html and 
https://clang.llvm.org/docs/SourceBasedCodeCoverage.html

I suspect that this patch will need to get split up into smaller parts. While 
it is very useful to see everything at once, it may be better to use this as a 
kind of support for the RFC, and then split off the implementation into smaller 
independently reviewable parts, even if the full functionality isn't useable 
until the last patch lands. For example the LLVM changes could be looked at and 
accepted prior to clang.

Will try and look and some more parts next week, my apologies I can't go 
through it all at once.




Comment at: clang/lib/CodeGen/CodeGenFunction.h:1522
+  /// Used by MC/DC to track the execution state of a boolean expression.
+  Address MCDCTempAddr = Address::invalid();
+

Would MCDCTrackingStateAddr be more appropriate? It could save people some time 
tracking back up to the comment to work out what Temp is being used for.



Comment at: clang/lib/CodeGen/CodeGenFunction.h:1542
 
+  bool isMCDCCoverageEnabled() {
+return (CGM.getCodeGenOpts().hasProfileClangInstr() &&

Can this be `bool isMCDCCoverageEnabled() const`



Comment at: clang/lib/CodeGen/CodeGenFunction.h:1554
+  MCDCTempAddr = CreateIRTemp(getContext().UnsignedIntTy, "mcdc.addr");
+  return;
+}

Is the return necessary here?



Comment at: clang/lib/CodeGen/CodeGenFunction.h:1558
+
+  bool isBinaryLogicalOp(const Expr *S) {
+const BinaryOperator *BOp = dyn_cast(S->IgnoreParens());

This looks like it doesn't need to be a member function? I can appreciate that 
here may be the most convenient place to  put it though.



Comment at: clang/lib/CodeGen/CodeGenPGO.cpp:164
+  /// The next bitmask byte index to assign.
+  unsigned NextBitmask;
+  /// The map of statements to bitmask coverage objects.

perhaps worth including index in the name, for example `NextBitmaskIdx` . Will 
help prevent confusion as to whether this is an index or an actual bitmask 
(comment does say, but harder to see from reading the code).



Comment at: clang/lib/CodeGen/CodeGenPGO.cpp:979
+  // set it to zero.
+  unsigned MCDCMaxConditions = 0;
+  if (CGM.getCodeGenOpts().MCDCCoverage)

`unsigned MCDCMaxConditions = (CGM.getCodeGenOpts().MCDCCoverage) ? 
CGM.getCodeGenOpts().MCDCMaxConditionCount : 0;`

Purely subjective opinion on my part though.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:102
 
+  unsigned Mask = 0;
+  unsigned Conditions = 0;

Worth a comment that these are associated with MCDC?



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:662
+  /// Is this a logical-AND operation?
+  bool isLAnd(const BinaryOperator *E) { return E->getOpcode() == BO_LAnd; }
+

Could this be a free function? If not then could it be `const`? 



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:718
+  void pushAndAssignIDs(const BinaryOperator *E) {
+if (CGM.getCodeGenOpts().MCDCCoverage) {
+  // If binary expression is disqualified, don't do mapping.

Would it be better to invert the condition and early exit? This would save a 
level of indentation for the rest of the function.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:759
+unsigned TotalConds = 0;
+if (CGM.getCodeGenOpts().MCDCCoverage) {
+  // Pop Stmt from 'NestLevel' stack.

Would it be better to invert the condition and early exit? This would save a 
level of indentation for the rest of the function.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:924
+   false)) {
+if (!CoverageMapping)
+  D.Diag(clang::diag::err_drv_argument_only_allowed_with)

Maybe able to simplify this to `if 
(Args.hasFlag(options::OPT_fcoverage_mapping)` and remove `CoverageMapping`. I 
think that in the case of `!ProfileGenerateArg` there are several options so it 
needed to be a variable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136385/new/

https://reviews.llvm.org/D136385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D133109: [LLVM][ARM] Remove options for armv2, 2A, 3 and 3M

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

LGTM. GCC no longer supports Arm architecture prior to v4 so it is likely the 
alternative of adding support for v3 is not worth it. The only Arm machines 
running v2 are likely to be the Acorn Archimedes family, these have their own 
object file format so are unlikely to benefit from LLVM support.

Agree that the openmp reference looks harmless.

Will be worth waiting to see if Nick has any objections.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133109/new/

https://reviews.llvm.org/D133109

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114124: [clang][ARM] only check -mtp=cp15 for non-asm sources

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

LGTM too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114124/new/

https://reviews.llvm.org/D114124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

LGTM, thanks for the update.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114116/new/

https://reviews.llvm.org/D114116

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Apologies, missed a couple of small things out. Otherwise looks good to me.




Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:150
 
+// The backend does not have support for hard thread pointers when targeting
+// Thumb1.

peter.smith wrote:
> Will be worth saying `CP15 C13 ThreadID` somewhere to make it easier to look 
> up in the documentation.
Now we're permitting anything with the co-processor instructions I think the 
comment would be better expressed as:
``` We follow GCC and support when the backend has support for the MRC/MCR 
instructions that are used to set the hard thread pointer ("CP15 C13 //Thread 
id").```



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:155
+  llvm::ARM::ArchKind AK = llvm::ARM::parseArch(Triple.getArchName());
+  return Ver >= 7 || AK == llvm::ARM::ArchKind::ARMV6T2 ||
+ (Ver == 6 && Triple.isARM());

peter.smith wrote:
> nickdesaulniers wrote:
> > peter.smith wrote:
> > > nickdesaulniers wrote:
> > > > ardb wrote:
> > > > > peter.smith wrote:
> > > > > > Are we restricting based on whether the threadid register is 
> > > > > > present or whether the instructions are available to access the 
> > > > > > cp15 registers?
> > > > > > 
> > > > > > If we're going by whether the CPU has the register present then it 
> > > > > > will be
> > > > > > * A and R profile (not M profile, even the ones that have Thumb2)
> > > > > > * V6K (includes ARM1176 but not ARM1156t2-s which has Thumb-2!) and 
> > > > > > V7+ (A and R profile)
> > > > > > 
> > > > > > If we're going by the instructions to write to CP15 then it is:
> > > > > > * Arm state (everything)
> > > > > > * Thumb2 (v7 + v6t2)
> > > > > > 
> > > > > > The above seems to be a blend of the two. Is it worth choosing one 
> > > > > > form or the other? GCC seems to use the latter. I guess using this 
> > > > > > option falls into the I know what I'm doing area that accessing 
> > > > > > named system registers comes into. If the kernel supports it the 
> > > > > > stricter version may help catch more mistakes though.
> > > > > > 
> > > > > > The v7 A/R Arm ARM 
> > > > > > https://developer.arm.com/documentation/ddi0403/ed
> > > > > > has in `D12.7.21 CP15 c13, Context ID support`
> > > > > > ``` An ARMv6K implementation requires the Software Thread ID 
> > > > > > registers described in VMSA CP15 c13
> > > > > > register summary, Process, context and thread ID registers on page 
> > > > > > B3-1474. ```
> > > > > > 
> > > > > > The Arm 1156-s (the only v6t2 processor) TRM 
> > > > > > https://developer.arm.com/documentation/ddi0338/g/system-control-coprocessor/system-control-processor-registers/c13--process-id-register?lang=en
> > > > > >  which shows only one process ID register under opcode 1 accessed 
> > > > > > via:
> > > > > > ```
> > > > > > MRC p15, 0, , c13, c0, 1 ;Read Process ID Register
> > > > > > ```
> > > > > > Whereas the ThreadID register is opcode 3 on CPUs that are v6k and 
> > > > > > v7.
> > > > > The primary reason for tightening these checks was to avoid an assert 
> > > > > in the backend when using -mtp=cp15 with a Thumb1 target, so I'd say 
> > > > > this is more about whether the ISA has the opcode to begin with, 
> > > > > rather than whether CPU x implements it or not.
> > > > > Arm state (everything)
> > > > 
> > > > 
> > > > Does that mean that `-march=armv5 -marm` has access/support for "CP15 
> > > > C13 ThreadID" access?
> > > The co-processor instructions are the same, but the effect of the 
> > > instructions will depend on the CPU. For example on armv5 we can write 
> > > the instruction to read/write to CP15 C13 with the ThreadID opcode. 
> > > However on no armv5 implementation will the CP15 C13 have a Thread ID 
> > > register. 
> > > 
> > > The more complex stricter check will make sure that the implementations 
> > > have the ThreadID register. As Ard mentions, the GCC intent seems to be 
> > > whether the instruction is encodable rather than check what the CPU 
> > > supports. 
> > I think this thread is resolved, so marking as done. Please reopen it 
> > though if I misunderstood.
> I think the discussion about whether the MRC/MCR instructions are available 
> is closed. I think the expression isn't quite right as > 7 will permit 
> v8-m.baseline, which is the evolution of cortex-m0.
> 
> To match GCC I think you want the equivalent of ARM || 
> hasThumb2Instructions() 
> 
> I think you can do that with the equivalent of 
> ArmTargetInfo::supportsThumb2() from lib/Basic/Targets/ARM.cpp
> ```
> bool ARMTargetInfo::supportsThumb2() const {
>   return CPUAttr.equals("6T2") ||
>  (ArchVersion >= 7 && !CPUAttr.equals("8M_BASE"));
> }
> ```
> Where CPUAttr.equals("8M_BASE") is equivalent to 
> `llvm::ARM::ArchKind::ARMV8MBaseline` so I think you can use
> ```
> return Triple.isARM() || (Ver >= 7 && AK != 
> llvm::ARM::ArchKind::ARMV8MBaseline);
> ```
> 
> ARMV6K and 

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I've made a suggestion to disallow v8-m.baseline (does not have Thumb 2 but has 
number > 7) and to simplify the expression.




Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:155
+  llvm::ARM::ArchKind AK = llvm::ARM::parseArch(Triple.getArchName());
+  return Ver >= 7 || AK == llvm::ARM::ArchKind::ARMV6T2 ||
+ (Ver == 6 && Triple.isARM());

nickdesaulniers wrote:
> peter.smith wrote:
> > nickdesaulniers wrote:
> > > ardb wrote:
> > > > peter.smith wrote:
> > > > > Are we restricting based on whether the threadid register is present 
> > > > > or whether the instructions are available to access the cp15 
> > > > > registers?
> > > > > 
> > > > > If we're going by whether the CPU has the register present then it 
> > > > > will be
> > > > > * A and R profile (not M profile, even the ones that have Thumb2)
> > > > > * V6K (includes ARM1176 but not ARM1156t2-s which has Thumb-2!) and 
> > > > > V7+ (A and R profile)
> > > > > 
> > > > > If we're going by the instructions to write to CP15 then it is:
> > > > > * Arm state (everything)
> > > > > * Thumb2 (v7 + v6t2)
> > > > > 
> > > > > The above seems to be a blend of the two. Is it worth choosing one 
> > > > > form or the other? GCC seems to use the latter. I guess using this 
> > > > > option falls into the I know what I'm doing area that accessing named 
> > > > > system registers comes into. If the kernel supports it the stricter 
> > > > > version may help catch more mistakes though.
> > > > > 
> > > > > The v7 A/R Arm ARM https://developer.arm.com/documentation/ddi0403/ed
> > > > > has in `D12.7.21 CP15 c13, Context ID support`
> > > > > ``` An ARMv6K implementation requires the Software Thread ID 
> > > > > registers described in VMSA CP15 c13
> > > > > register summary, Process, context and thread ID registers on page 
> > > > > B3-1474. ```
> > > > > 
> > > > > The Arm 1156-s (the only v6t2 processor) TRM 
> > > > > https://developer.arm.com/documentation/ddi0338/g/system-control-coprocessor/system-control-processor-registers/c13--process-id-register?lang=en
> > > > >  which shows only one process ID register under opcode 1 accessed via:
> > > > > ```
> > > > > MRC p15, 0, , c13, c0, 1 ;Read Process ID Register
> > > > > ```
> > > > > Whereas the ThreadID register is opcode 3 on CPUs that are v6k and v7.
> > > > The primary reason for tightening these checks was to avoid an assert 
> > > > in the backend when using -mtp=cp15 with a Thumb1 target, so I'd say 
> > > > this is more about whether the ISA has the opcode to begin with, rather 
> > > > than whether CPU x implements it or not.
> > > > Arm state (everything)
> > > 
> > > 
> > > Does that mean that `-march=armv5 -marm` has access/support for "CP15 C13 
> > > ThreadID" access?
> > The co-processor instructions are the same, but the effect of the 
> > instructions will depend on the CPU. For example on armv5 we can write the 
> > instruction to read/write to CP15 C13 with the ThreadID opcode. However on 
> > no armv5 implementation will the CP15 C13 have a Thread ID register. 
> > 
> > The more complex stricter check will make sure that the implementations 
> > have the ThreadID register. As Ard mentions, the GCC intent seems to be 
> > whether the instruction is encodable rather than check what the CPU 
> > supports. 
> I think this thread is resolved, so marking as done. Please reopen it though 
> if I misunderstood.
I think the discussion about whether the MRC/MCR instructions are available is 
closed. I think the expression isn't quite right as > 7 will permit 
v8-m.baseline, which is the evolution of cortex-m0.

To match GCC I think you want the equivalent of ARM || hasThumb2Instructions() 

I think you can do that with the equivalent of ArmTargetInfo::supportsThumb2() 
from lib/Basic/Targets/ARM.cpp
```
bool ARMTargetInfo::supportsThumb2() const {
  return CPUAttr.equals("6T2") ||
 (ArchVersion >= 7 && !CPUAttr.equals("8M_BASE"));
}
```
Where CPUAttr.equals("8M_BASE") is equivalent to 
`llvm::ARM::ArchKind::ARMV8MBaseline` so I think you can use
```
return Triple.isARM() || (Ver >= 7 && AK != 
llvm::ARM::ArchKind::ARMV8MBaseline);
```

ARMV6K and ARMV6KZ can only MRC and MCR in ARM mode so are covered by 
Triple.isARM().




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114116/new/

https://reviews.llvm.org/D114116

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for ARMv6 non-thumb cases

[Peter Smith via Phabricator via cfe-commits]

peter.smith added inline comments.



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:155
+  llvm::ARM::ArchKind AK = llvm::ARM::parseArch(Triple.getArchName());
+  return Ver >= 7 || AK == llvm::ARM::ArchKind::ARMV6T2 ||
+ (Ver == 6 && Triple.isARM());

nickdesaulniers wrote:
> ardb wrote:
> > peter.smith wrote:
> > > Are we restricting based on whether the threadid register is present or 
> > > whether the instructions are available to access the cp15 registers?
> > > 
> > > If we're going by whether the CPU has the register present then it will be
> > > * A and R profile (not M profile, even the ones that have Thumb2)
> > > * V6K (includes ARM1176 but not ARM1156t2-s which has Thumb-2!) and V7+ 
> > > (A and R profile)
> > > 
> > > If we're going by the instructions to write to CP15 then it is:
> > > * Arm state (everything)
> > > * Thumb2 (v7 + v6t2)
> > > 
> > > The above seems to be a blend of the two. Is it worth choosing one form 
> > > or the other? GCC seems to use the latter. I guess using this option 
> > > falls into the I know what I'm doing area that accessing named system 
> > > registers comes into. If the kernel supports it the stricter version may 
> > > help catch more mistakes though.
> > > 
> > > The v7 A/R Arm ARM https://developer.arm.com/documentation/ddi0403/ed
> > > has in `D12.7.21 CP15 c13, Context ID support`
> > > ``` An ARMv6K implementation requires the Software Thread ID registers 
> > > described in VMSA CP15 c13
> > > register summary, Process, context and thread ID registers on page 
> > > B3-1474. ```
> > > 
> > > The Arm 1156-s (the only v6t2 processor) TRM 
> > > https://developer.arm.com/documentation/ddi0338/g/system-control-coprocessor/system-control-processor-registers/c13--process-id-register?lang=en
> > >  which shows only one process ID register under opcode 1 accessed via:
> > > ```
> > > MRC p15, 0, , c13, c0, 1 ;Read Process ID Register
> > > ```
> > > Whereas the ThreadID register is opcode 3 on CPUs that are v6k and v7.
> > The primary reason for tightening these checks was to avoid an assert in 
> > the backend when using -mtp=cp15 with a Thumb1 target, so I'd say this is 
> > more about whether the ISA has the opcode to begin with, rather than 
> > whether CPU x implements it or not.
> > Arm state (everything)
> 
> 
> Does that mean that `-march=armv5 -marm` has access/support for "CP15 C13 
> ThreadID" access?
The co-processor instructions are the same, but the effect of the instructions 
will depend on the CPU. For example on armv5 we can write the instruction to 
read/write to CP15 C13 with the ThreadID opcode. However on no armv5 
implementation will the CP15 C13 have a Thread ID register. 

The more complex stricter check will make sure that the implementations have 
the ThreadID register. As Ard mentions, the GCC intent seems to be whether the 
instruction is encodable rather than check what the CPU supports. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114116/new/

https://reviews.llvm.org/D114116

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for ARMv6 non-thumb cases

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Apologies had to go on a bit of a dive through the documentation. I've put some 
comments inline and some links to other documents that may help.




Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:150
 
+// The backend does not have support for hard thread pointers when targeting
+// Thumb1.

Will be worth saying `CP15 C13 ThreadID` somewhere to make it easier to look up 
in the documentation.



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:155
+  llvm::ARM::ArchKind AK = llvm::ARM::parseArch(Triple.getArchName());
+  return Ver >= 7 || AK == llvm::ARM::ArchKind::ARMV6T2 ||
+ (Ver == 6 && Triple.isARM());

Are we restricting based on whether the threadid register is present or whether 
the instructions are available to access the cp15 registers?

If we're going by whether the CPU has the register present then it will be
* A and R profile (not M profile, even the ones that have Thumb2)
* V6K (includes ARM1176 but not ARM1156t2-s which has Thumb-2!) and V7+ (A and 
R profile)

If we're going by the instructions to write to CP15 then it is:
* Arm state (everything)
* Thumb2 (v7 + v6t2)

The above seems to be a blend of the two. Is it worth choosing one form or the 
other? GCC seems to use the latter. I guess using this option falls into the I 
know what I'm doing area that accessing named system registers comes into. If 
the kernel supports it the stricter version may help catch more mistakes though.

The v7 A/R Arm ARM https://developer.arm.com/documentation/ddi0403/ed
has in `D12.7.21 CP15 c13, Context ID support`
``` An ARMv6K implementation requires the Software Thread ID registers 
described in VMSA CP15 c13
register summary, Process, context and thread ID registers on page B3-1474. ```

The Arm 1156-s (the only v6t2 processor) TRM 
https://developer.arm.com/documentation/ddi0338/g/system-control-coprocessor/system-control-processor-registers/c13--process-id-register?lang=en
 which shows only one process ID register under opcode 1 accessed via:
```
MRC p15, 0, , c13, c0, 1 ;Read Process ID Register
```
Whereas the ThreadID register is opcode 3 on CPUs that are v6k and v7.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114116/new/

https://reviews.llvm.org/D114116

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112768: [ARM] implement support for TLS register based stack protector

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a reviewer: ostannard.
peter.smith added a comment.

Adding ostannard to reviewers list. I'm going to be on vacation next week and 
Oliver is more familiar with this area than I am.

To prevent the option in Clang for targets that don't support Thumb2 it may be 
worth looking into clang/lib/Basic/Targets/ARM.cpp for example 
https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/ARM.cpp#L174


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112768/new/

https://reviews.llvm.org/D112768

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112600: [ARM] Use hardware TLS register in Thumb2 mode when -mtp=cp15 is passed

[Peter Smith via Phabricator via cfe-commits]

peter.smith added inline comments.



Comment at: clang/test/CodeGen/arm-tphard.c:10
+}
+

nickdesaulniers wrote:
> ardb wrote:
> > nickdesaulniers wrote:
> > > Let's make this a test under llvm/test/CodeGen/, using IR:
> > > ```
> > > ; RUN: llc --mtriple=armv7-linux-gnueabihf -o - %s | FileCheck %s
> > > ; RUN: llc --mtriple=thumbv7-linux-gnu -o - %s | FileCheck %s
> > > define dso_local i8* @tphard() "target-features"="+read-tp-hard" {
> > > // CHECK-NOT: __aeabi_read_tp
> > >   %1 = tail call i8* @llvm.thread.pointer()
> > >   ret i8* %1
> > > }
> > > 
> > > declare i8* @llvm.thread.pointer()
> > > ```
> > > 
> > > Let's make this a test under llvm/test/CodeGen/, using IR:
> > 
> > Why is that better?
> > 
> > > ```
> > > ; RUN: llc --mtriple=armv7-linux-gnueabihf -o - %s | FileCheck %s
> > > ; RUN: llc --mtriple=thumbv7-linux-gnu -o - %s | FileCheck %s
> > 
> > Are you sure using this triple forces generation of Thumb2 code? It didn't 
> > seem to when I tried.
> > 
> > > define dso_local i8* @tphard() "target-features"="+read-tp-hard" {
> > > // CHECK-NOT: __aeabi_read_tp
> > 
> > Are you sure __aeabi_read_tp will appear in the IR for soft TP?
> > 
> > >   %1 = tail call i8* @llvm.thread.pointer()
> > >   ret i8* %1
> > > }
> > > 
> > > declare i8* @llvm.thread.pointer()
> > > ```
> > > 
> > 
> > 
> > Why is that better?
> 
> Because the front-end isn't really involved in this back end code gen bug, so 
> we should just be testing the back end.
> 
> > Are you sure using this triple forces generation of Thumb2 code? It didn't 
> > seem to when I tried.
> 
> Good question; I guess for thumb2 there's no command line flag passed to the 
> compiler that says "I would like thumb[1] as opposed to thumb2?"  Maybe 
> @peter.smith can provide some color on that? Is it simply armv7 for thumb2 vs 
> v6 for thumb[1], or something else?
> 
> > Are you sure __aeabi_read_tp will appear in the IR for soft TP?
> 
> `__aeabi_read_tp` will never appear in the IR (unless someone explicitly 
> called it`. Instead, we're testing that the intrinsic 
> (`@llvm.thread.pointer`) is lowered to either that libcall or `mrc`.  
> `__aeabi_read_tp` may appear in the object file or assembler code generated 
> from that intrinsic call.
> 
> But also, it looks like there's already coverage for `__aeabi_read_tp` being 
> generated for soft TP. See also llvm/test/CodeGen/ARM/readtp.ll. There's also 
> thread_pointer.ll in that same dir (and a few more tests mentioning 
> `__aeabi_read_tp` that all look like candidates to add these tests to rather 
> than creating a new test file, perhaps.
> > Are you sure using this triple forces generation of Thumb2 code? It didn't 
> > seem to when I tried.
> 
> Good question; I guess for thumb2 there's no command line flag passed to the 
> compiler that says "I would like thumb[1] as opposed to thumb2?"  Maybe 
> @peter.smith can provide some color on that? Is it simply armv7 for thumb2 vs 
> v6 for thumb[1], or something else?

As I understand it, "Thumb2 Technology" to give it the marketing name is still 
the Thumb (T32) ISA, it just has access to a lot more instructions than Thumb. 
In the backend this means that the compiler is free to select instructions with 
the Thumb2 predicate. It doesn't have to if there is an equivalent 2-byte sized 
Thumb instruction that does the job.

In theory to get Thumb only code -march=armv6 might work, armv6 is the 
intersection of the A, R and M profiles that include no Thumb 2.

The architecture for Thumb 2 is close to v7+ although like most things Arm it 
is more complicated:
* Armv6k as implemented by one CPU Arm1165t2-s (which I wouldn't expect to see 
running Linux). All other Arm v6 CPUs including the Arm 1176jzf-s (original 
Raspberry Pi) do not have Thumb 2.
* All Arm v7 CPUs have Thumb 2, including M, R and A profiles.
* All Arm v8 R and A profile CPUs have Thumb 2 as do Armv8-M.mainline from M 
profile. The Armv8-M.baseline CPUs do not (these are the smallest 
microcontrollers). 




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112600/new/

https://reviews.llvm.org/D112600

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112600: [ARM] Use hardware TLS register in Thumb2 mode when -mtp=cp15 is passed

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

LGTM as this as CP15 can be used on architecture v6k and above, which maps to 
IsThumb2.

As an aside from this patch, the Arm state could be considered too permissive 
as it will permit -mtls=cp15 on architectures that wouldn't have the 
coprocessor register like arm7tdmi, although GCC also permits this so I guess 
we're in the set this option with care territory.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112600/new/

https://reviews.llvm.org/D112600

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111134: Add basic aarch64-none-elf bare metal driver.

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

LGTM thanks for the update. This looks like it follows the same format as the 
other -none-elf toolchains, and AArch64 can benefit from the bare-metal 
driver for easier access to LLD. Will be worth waiting a few days to see if 
there is any other feedback.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D34/new/

https://reviews.llvm.org/D34

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45961: [MC] Add MCSubtargetInfo to MCAlignFragment

[Peter Smith via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG5e71839f7793: [MC] Add MCSubtargetInfo to MCAlignFragment 
(authored by psmith).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D45961?vs=370923=371074#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D45961/new/

https://reviews.llvm.org/D45961

Files:
  clang/tools/driver/cc1as_main.cpp
  llvm/include/llvm/MC/MCELFStreamer.h
  llvm/include/llvm/MC/MCFragment.h
  llvm/include/llvm/MC/MCObjectStreamer.h
  llvm/include/llvm/MC/MCStreamer.h
  llvm/include/llvm/MC/MCWinCOFFStreamer.h
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/MC/ConstantPools.cpp
  llvm/lib/MC/MCAsmStreamer.cpp
  llvm/lib/MC/MCELFStreamer.cpp
  llvm/lib/MC/MCObjectStreamer.cpp
  llvm/lib/MC/MCParser/AsmParser.cpp
  llvm/lib/MC/MCParser/MasmParser.cpp
  llvm/lib/MC/MCStreamer.cpp
  llvm/lib/MC/MCWinCOFFStreamer.cpp
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/lib/Target/ARM/ARMAsmPrinter.cpp
  llvm/lib/Target/ARM/ARMMCInstLower.cpp
  llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
  llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
  llvm/lib/Target/Hexagon/AsmParser/HexagonAsmParser.cpp
  llvm/lib/Target/Hexagon/HexagonAsmPrinter.cpp
  llvm/lib/Target/Hexagon/HexagonTargetStreamer.h
  llvm/lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp
  llvm/lib/Target/Mips/MipsAsmPrinter.cpp
  llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp
  llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFStreamer.cpp
  llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
  llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
  llvm/lib/Target/X86/X86MCInstLower.cpp
  llvm/tools/llvm-mc/Disassembler.cpp
  llvm/tools/llvm-mc/llvm-mc.cpp
  llvm/tools/llvm-ml/Disassembler.cpp
  llvm/unittests/DebugInfo/DWARF/DWARFExpressionCopyBytesTest.cpp

Index: llvm/unittests/DebugInfo/DWARF/DWARFExpressionCopyBytesTest.cpp
===
--- llvm/unittests/DebugInfo/DWARF/DWARFExpressionCopyBytesTest.cpp
+++ llvm/unittests/DebugInfo/DWARF/DWARFExpressionCopyBytesTest.cpp
@@ -128,7 +128,7 @@
   SmallString<0> Storage;
   raw_svector_ostream VecOS(Storage);
   StreamerContext C = createStreamer(VecOS);
-  C.Streamer->InitSections(false);
+  C.Streamer->initSections(false, *STI);
   MCSection *Section = C.MOFI->getTextSection();
   Section->setHasInstructions(true);
   C.Streamer->SwitchSection(Section);
Index: llvm/tools/llvm-ml/Disassembler.cpp
===
--- llvm/tools/llvm-ml/Disassembler.cpp
+++ llvm/tools/llvm-ml/Disassembler.cpp
@@ -152,7 +152,7 @@
   }
 
   // Set up initial section manually here
-  Streamer.InitSections(false);
+  Streamer.initSections(false, STI);
 
   bool ErrorOccurred = false;
 
Index: llvm/tools/llvm-mc/llvm-mc.cpp
===
--- llvm/tools/llvm-mc/llvm-mc.cpp
+++ llvm/tools/llvm-mc/llvm-mc.cpp
@@ -571,7 +571,7 @@
 MCOptions.MCIncrementalLinkerCompatible,
 /*DWARFMustBeAtTheEnd*/ false));
 if (NoExecStack)
-  Str->InitSections(true);
+  Str->initSections(true, *STI);
   }
 
   // Use Assembler information for parsing.
Index: llvm/tools/llvm-mc/Disassembler.cpp
===
--- llvm/tools/llvm-mc/Disassembler.cpp
+++ llvm/tools/llvm-mc/Disassembler.cpp
@@ -156,7 +156,7 @@
   }
 
   // Set up initial section manually here
-  Streamer.InitSections(false);
+  Streamer.initSections(false, STI);
 
   bool ErrorOccurred = false;
 
Index: llvm/lib/Target/X86/X86MCInstLower.cpp
===
--- llvm/lib/Target/X86/X86MCInstLower.cpp
+++ llvm/lib/Target/X86/X86MCInstLower.cpp
@@ -1718,7 +1718,7 @@
   // First we emit the label and the jump.
   auto CurSled = OutContext.createTempSymbol("xray_event_sled_", true);
   OutStreamer->AddComment("# XRay Custom Event Log");
-  OutStreamer->emitCodeAlignment(2);
+  OutStreamer->emitCodeAlignment(2, ());
   OutStreamer->emitLabel(CurSled);
 
   // Use a two-byte `jmp`. This version of JMP takes an 8-bit relative offset as
@@ -1814,7 +1814,7 @@
   // First we emit the label and the jump.
   auto CurSled = OutContext.createTempSymbol("xray_typed_event_sled_", true);
   OutStreamer->AddComment("# XRay Typed Event Log");
-  OutStreamer->emitCodeAlignment(2);
+  OutStreamer->emitCodeAlignment(2, ());
   OutStreamer->emitLabel(CurSled);
 
   // Use a two-byte `jmp`. This version of JMP takes an 8-bit relative offset as
@@ -1916,7 +1916,7 @@
   //   call// 5 bytes
   //
   auto CurSled = OutContext.createTempSymbol("xray_sled_", true);
-  OutStreamer->emitCodeAlignment(2);
+  OutStreamer->emitCodeAlignment(2, ());
   

[PATCH] D101873: [clang] Support clang -fpic -fno-semantic-interposition for AArch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Doing this on 32-bit Arm would make me nervous as STT_FUNC symbols encode the 
state of Arm/Thumb in the bottom bit, but STT_NOTYPE symbols do not. In 
principle it could be done but extra care would have to be taken to make sure 
no state changes were required. For example caller and callee would need to be 
in the same state. I'm not entirely sure that LLD's current range-extension 
thunks would work to STT_NOTYPE symbols as they use BX IP, which would always 
state change to Arm due to the STT_NOTYPE symbol having the bottom bit clear. 
This is fixable but is the additional complexity and possible fragility of 
older tools worth it?

https://github.com/ARM-software/abi-aa/blob/main/aaelf32/aaelf32.rst#554symbol-names


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101873/new/

https://reviews.llvm.org/D101873

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101873: [clang] Support clang -fpic -fno-semantic-interposition for AArch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

LGTM as this is opt in with a command line option.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101873/new/

https://reviews.llvm.org/D101873

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101873: [clang] Support clang -fpic -fno-semantic-interposition for AArch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Thanks for the update.

With the clarification that this isn't breaking aarch64 long range thunks now, 
and we are not considering Arm then I'm happy for this to happen if the user 
opts in with -fno-semantic-interposition. I think the longer term question, 
outside of the scope of this review, about whether -fno-semantic-interposition 
should be the default is probably one for llvm-dev.

In D101873#2742660 , @MaskRay wrote:

> In D101873#2741903 , @peter.smith 
> wrote:
>
>> In D101873#2741299 , @MaskRay 
>> wrote:
>>
>>> https://gist.github.com/MaskRay/2d4dfcfc897341163f734afb59f689c6 has more 
>>> information about -fno-semantic-interposition.
>>>
 Can Clang default to -fno-semantic-interposition?
>>>
>>> I think we can probably make non-x86 default to -fno-semantic-interposition 
>>> (dso_local inference, given D72197 . x86 
>>> may find default -fno-semantic-interposition too aggressive.
>>
>> Thanks for the link, and the explanation that -fno-semantic-interposition is 
>> not the default.
>>
>> I'm not (yet) convinced that we could make -fno-semantic-interposition the 
>> default, primarily due to data and not functions, I agree that 
>> interpositioning functions is rarely used. For data the classic example for 
>> symbol-interposition was errno, a shared library can't know if any other 
>> library or executable will define it so it must define, but it must use only 
>> one value for the definition. I'm not sure if that still holds in today's 
>> environment with shared C libraries used by practically everything but I 
>> think the principle still applies.
>
> errno needs to be thread-local. C11 7.5.2 says "and errno which expands to a 
> modifiable lvalue that has type int and thread local storage duration, the 
> value of which is set to a positive error number by several library 
> functions."
> Do you mean that in some environment it may be defined in more than one 
> shared object?

In the general case it is multiple shared libraries include the same static 
library that has a global variable, in the normal rules only one of these 
globals will be used, wheras with -fno-semantic-interposition they will all use 
individual copies. I don't think that this is common as it is not considered 
good design, it is just an example of how some programs could be broken in 
subtle ways if the default were changed.

>> Looking at the gist I've got one concern for AArch64 and Arm. The ABI relies 
>> on thunks which are only defined for symbols of type STT_FUNC. Changing 
>> branches to STT_FUNC to STT_SECTION will break long range thunks on AArch64 
>> and interworking for Arm (there is a possibility that the bottom bit for 
>> STT_FUNC may get used in the future for AArch64 as well). This is solvable 
>> by keeping the local label and setting STT_FUNC on it.
>
> I'll unlikely touch 32-bit arm.
>
> For aarch64, aaelf64/aaelf64.rst says
>
>   A linker may use a veneer (a sequence of instructions) to implement a 
> relocated branch if the relocation is either
>   
>   ``R__CALL26``, ``R__JUMP26`` or ``R__PLT32`` and:
>   
>   - The target symbol has type ``STT_FUNC``.
>   
>   - Or, the target symbol and relocated place are in separate sections input 
> to the linker.
>   
>   - Or, the target symbol is undefined (external to the link unit).
>
> If `bl .Lhigh_target$local` and `.Lhigh_target$local` are in the same 
> section, the fixup is resolved at assembly time;
> otherwise, they are in separate sections and satisfy the ABI requirement.
>
> I just changed `bl high_target` in `test/lld/ELF/aarch64-thunk-script.s` and 
> noticed that both GNU ld and ld.lld can produce a thunk, regardless of the 
> symbol type.

OK, so it looks like the "Or, the target symbol and relocated place are in 
separate sections input to the linker." can cover AArch64.

An area I didn't want to mention earlier as there is no guarantee it will be 
part of the architecture, or the ABI is Morello. This introduces capabilities 
into AArch64 
https://github.com/ARM-software/abi-aa/blob/main/aaelf64-morello/aaelf64-morello.rst#414symbol-values
 with an eye to the future where this might be significant. I realise that we 
can't be hostage to a future that might not come to pass and there can always 
be "turn fno-semantic-interposition off when Morello is selected" but my 
instinct is to be cautious as I don't want to make Morello even more difficult 
than it already is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101873/new/

https://reviews.llvm.org/D101873

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101873: [clang] Support clang -fpic -fno-semantic-interposition for AArch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In D101873#2741299 , @MaskRay wrote:

> https://gist.github.com/MaskRay/2d4dfcfc897341163f734afb59f689c6 has more 
> information about -fno-semantic-interposition.
>
>> Can Clang default to -fno-semantic-interposition?
>
> I think we can probably make non-x86 default to -fno-semantic-interposition 
> (dso_local inference, given D72197 . x86 may 
> find default -fno-semantic-interposition too aggressive.

Thanks for the link, and the explanation that -fno-semantic-interposition is 
not the default.

I'm not (yet) convinced that we could make -fno-semantic-interposition the 
default, primarily due to data and not functions, I agree that interpositioning 
functions is rarely used. For data the classic example for symbol-interposition 
was errno, a shared library can't know if any other library or executable will 
define it so it must define, but it must use only one value for the definition. 
I'm not sure if that still holds in today's environment with shared C libraries 
used by practically everything but I think the principle still applies.

Looking at the gist I've got one concern for AArch64 and Arm. The ABI relies on 
thunks which are only defined for symbols of type STT_FUNC. Changing branches 
to STT_FUNC to STT_SECTION will break long range thunks on AArch64 and 
interworking for Arm (there is a possibility that the bottom bit for STT_FUNC 
may get used in the future for AArch64 as well). This is solvable by keeping 
the local label and setting STT_FUNC on it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101873/new/

https://reviews.llvm.org/D101873

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101873: [clang] Support clang -fpic -fno-semantic-interposition for AArch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I've no comments on the code in D101872  , 
and D10873  they look reasonable to me. I 
guess it is down to whether this is the right thing to do or not.

Just to check my understanding:

- Clang defaults to -fno-semantic-interposition (GCC I believe has the opposite 
default fsemantic-interposition)
- With this change code in the same translation unit that defines the global 
will use a local alias for the global rather than accessing via the GOT, but 
the global will still be defined with default visibility.
- Symbol interposing is still permitted at link time so the global can be 
interposed, but as the code is using a local alias it will still use the 
original value.
- Without this chang clang would use fhalf-no-semantic-interposition which I 
believe permits some assumptions about symbol interpositioning such as 
resolving some short range assembly pc-relative references to a local alias. 
These would be out of range if the symbol were interposed anyway.

If I've got this right, particularly the default  then this makes me nervous 
about the default behaviour as it could silently break some existing code. If a 
user had to opt in explicitly with -fno-semantic-interposition then fair enough.

Can you let me know, if I'm being overly cautious here? For example are 
programs that would be affected by this already broken by the 
half-no-semantic-interpositioning anyway? Is symbol interpositioning so rare 
that the X86 version of this didn't break anything? Have I got the default of 
-fno-semantic-interpositioning wrong?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101873/new/

https://reviews.llvm.org/D101873

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100372: [Clang][ARM] Define __VFP_FP__ macro unconditionally

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added reviewers: compnerd, rengolin.
peter.smith added a comment.
This revision is now accepted and ready to land.

I think this is the right thing to do. GCC changed to unconditionally set the 
macro with https://gcc.gnu.org/legacy-ml/gcc-patches/2016-10/msg01025.html my 
understanding is that the macro means that floating point representation uses 
the VFP format and not the FPA format (predecessor to VFP, not supported by 
clang, found in very old systems), it does not mean that there is a VFP unit 
present.

see https://wiki.debian.org/ArmEabiPort for the mutually exclusive 
`__MAVERICK__` whcih refers to https://wiki.debian.org/ArmEabiMaverickCrunch 
the processor that had the FPA FPU.

I've set LGTM, and added a few more non-Arm people involved in the area for 
visibility, will be good to give them a chance to comment before committing.




Comment at: clang/lib/Basic/Targets/ARM.cpp:758
 
+  Builder.defineMacro("__VFP_FP__");
+

Worth a comment like "__VFP_FP__ means that the floating point format is VFP, 
not that a hardware FPU is present. The VFP format is the only one supported by 
clang."


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100372/new/

https://reviews.llvm.org/D100372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91760: [Driver] Default Generic_GCC aarch64 to use -fasynchronous-unwind-tables

[Peter Smith via Phabricator via cfe-commits]

psmith added a comment.

Radio silence so far; I think no news is good news applies in this case. I'm 
happy to say no objections.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91760/new/

https://reviews.llvm.org/D91760

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91760: [Driver] Default Generic_GCC aarch64 to use -fasynchronous-unwind-tables

[Peter Smith via Phabricator via cfe-commits]

psmith added a comment.

Personally I'm in favour of clang and gcc behaving the same way unless there is 
a good reason not to. I've shared the review internally to see if anyone has 
any concerns. May be worth informing the clang built linux community to see if 
they will need to make any alterations as a result.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91760/new/

https://reviews.llvm.org/D91760

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

[Peter Smith via Phabricator via cfe-commits]

psmith added a comment.

I agree it's not what I'd like. I'm not sure how to react to MaskRays comment. 
The top of the ClangCommandLineReference.rst says:

  ---
  NOTE: This file is automatically generated by running clang-tblgen
  -gen-opt-docs. Do not edit this file by hand!!
  ---

This uses Options.td to generate the file. I don't know how true this is 
anymore given that both files are checked in. They may have been separated. 
I've seen at least one instance in the log where the whole file has been 
regenerated.

I'm happy to revert the Options.td change if there is another way?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85239/new/

https://reviews.llvm.org/D85239

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

[Peter Smith via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
psmith marked an inline comment as done.
Closed by commit rG839d974ee0e4: [DOCS] Add more detail to stack protector 
documentation (authored by psmith).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85239/new/

https://reviews.llvm.org/D85239

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Driver/Options.td


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1801,10 +1801,15 @@
"as well as any calls to alloca or the taking of an address from a 
local variable">;
 def fstack_protector : Flag<["-"], "fstack-protector">, Group,
   HelpText<"Enable stack protectors for some functions vulnerable to stack 
smashing. "
-   "This uses a loose heuristic which considers functions vulnerable "
-   "if they contain a char (or 8bit integer) array or constant sized 
calls to "
-   "alloca, which are of greater size than ssp-buffer-size (default: 8 
bytes). "
-   "All variable sized calls to alloca are considered vulnerable">;
+   "This uses a loose heuristic which considers functions vulnerable 
if they "
+   "contain a char (or 8bit integer) array or constant sized calls to 
alloca "
+   ", which are of greater size than ssp-buffer-size (default: 8 
bytes). All "
+   "variable sized calls to alloca are considered vulnerable. A 
function with"
+   "a stack protector has a guard value added to the stack frame that 
is "
+   "checked on function exit. The guard value must be positioned in 
the "
+   "stack frame such that a buffer overflow from a vulnerable variable 
will "
+   "overwrite the guard value before overwriting the function's return 
"
+   "address. The reference stack guard value is stored in a global 
variable.">;
 def ftrivial_auto_var_init : Joined<["-"], "ftrivial-auto-var-init=">, 
Group,
   Flags<[CC1Option, CoreOption]>, HelpText<"Initialize trivial automatic stack 
variables: uninitialized (default)"
   " | pattern">, Values<"uninitialized,pattern">;
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -2136,7 +2136,7 @@
 
 .. option:: -fstack-protector, -fno-stack-protector
 
-Enable stack protectors for some functions vulnerable to stack smashing. This 
uses a loose heuristic which considers functions vulnerable if they contain a 
char (or 8bit integer) array or constant sized calls to alloca, which are of 
greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls 
to alloca are considered vulnerable
+Enable stack protectors for some functions vulnerable to stack smashing. This 
uses a loose heuristic which considers functions vulnerable if they contain a 
char (or 8bit integer) array or constant sized calls to alloca , which are of 
greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls 
to alloca are considered vulnerable. A function witha stack protector has a 
guard value added to the stack frame that is checked on function exit. The 
guard value must be positioned in the stack frame such that a buffer overflow 
from a vulnerable variable will overwrite the guard value before overwriting 
the function's return address. The reference stack guard value is stored in a 
global variable.
 
 .. option:: -fstack-protector-all
 


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1801,10 +1801,15 @@
"as well as any calls to alloca or the taking of an address from a local variable">;
 def fstack_protector : Flag<["-"], "fstack-protector">, Group,
   HelpText<"Enable stack protectors for some functions vulnerable to stack smashing. "
-   "This uses a loose heuristic which considers functions vulnerable "
-   "if they contain a char (or 8bit integer) array or constant sized calls to "
-   "alloca, which are of greater size than ssp-buffer-size (default: 8 bytes). "
-   "All variable sized calls to alloca are considered vulnerable">;
+   "This uses a loose heuristic which considers functions vulnerable if they "
+   "contain a char (or 8bit integer) array or constant sized calls to alloca "
+   ", which are of greater size than ssp-buffer-size (default: 8 bytes). All "
+   "variable sized calls to alloca are considered vulnerable. A function with"
+   "a stack protector has a guard value added to the stack frame that is "
+   "checked 

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

[Peter Smith via Phabricator via cfe-commits]

psmith marked an inline comment as done.
psmith added inline comments.



Comment at: clang/docs/ClangCommandLineReference.rst:2139
 
-Enable stack protectors for some functions vulnerable to stack smashing. This 
uses a loose heuristic which considers functions vulnerable if they contain a 
char (or 8bit integer) array or constant sized calls to alloca, which are of 
greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls 
to alloca are considered vulnerable
+Enable stack protectors for some functions vulnerable to stack smashing. This 
uses a loose heuristic which considers functions vulnerable if they contain a 
char (or 8bit integer) array or constant sized calls to alloca, which are of 
greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls 
to alloca are considered vulnerable. A function with a stack protector has a 
guard value added to the stack frame that is checked on function exit. The 
guard value must be positioned in the stack frame such that a buffer overflow 
from a vulnerable variable will overwrite to the guard value before overwriting 
the function's return address. The reference stack guard value is stored in a 
global variable.
 

probinson wrote:
> "overwrite to the guard variable" -> "overwrite the guard variable"
Thanks, now updated


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85239/new/

https://reviews.llvm.org/D85239

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

[Peter Smith via Phabricator via cfe-commits]

psmith updated this revision to Diff 283201.
psmith added a comment.
Herald added a subscriber: dang.

uploaded diff with both Options.td and ClangCommandLineReference.rst


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85239/new/

https://reviews.llvm.org/D85239

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Driver/Options.td


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1801,10 +1801,15 @@
"as well as any calls to alloca or the taking of an address from a 
local variable">;
 def fstack_protector : Flag<["-"], "fstack-protector">, Group,
   HelpText<"Enable stack protectors for some functions vulnerable to stack 
smashing. "
-   "This uses a loose heuristic which considers functions vulnerable "
-   "if they contain a char (or 8bit integer) array or constant sized 
calls to "
-   "alloca, which are of greater size than ssp-buffer-size (default: 8 
bytes). "
-   "All variable sized calls to alloca are considered vulnerable">;
+   "This uses a loose heuristic which considers functions vulnerable 
if they "
+   "contain a char (or 8bit integer) array or constant sized calls to 
alloca "
+   ", which are of greater size than ssp-buffer-size (default: 8 
bytes). All "
+   "variable sized calls to alloca are considered vulnerable. A 
function with"
+   "a stack protector has a guard value added to the stack frame that 
is "
+   "checked on function exit. The guard value must be positioned in 
the "
+   "stack frame such that a buffer overflow from a vulnerable variable 
will "
+   "overwrite the guard value before overwriting the function's return 
"
+   "address. The reference stack guard value is stored in a global 
variable.">;
 def ftrivial_auto_var_init : Joined<["-"], "ftrivial-auto-var-init=">, 
Group,
   Flags<[CC1Option, CoreOption]>, HelpText<"Initialize trivial automatic stack 
variables: uninitialized (default)"
   " | pattern">, Values<"uninitialized,pattern">;
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -2136,7 +2136,7 @@
 
 .. option:: -fstack-protector, -fno-stack-protector
 
-Enable stack protectors for some functions vulnerable to stack smashing. This 
uses a loose heuristic which considers functions vulnerable if they contain a 
char (or 8bit integer) array or constant sized calls to alloca, which are of 
greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls 
to alloca are considered vulnerable
+Enable stack protectors for some functions vulnerable to stack smashing. This 
uses a loose heuristic which considers functions vulnerable if they contain a 
char (or 8bit integer) array or constant sized calls to alloca , which are of 
greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls 
to alloca are considered vulnerable. A function witha stack protector has a 
guard value added to the stack frame that is checked on function exit. The 
guard value must be positioned in the stack frame such that a buffer overflow 
from a vulnerable variable will overwrite the guard value before overwriting 
the function's return address. The reference stack guard value is stored in a 
global variable.
 
 .. option:: -fstack-protector-all
 


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1801,10 +1801,15 @@
"as well as any calls to alloca or the taking of an address from a local variable">;
 def fstack_protector : Flag<["-"], "fstack-protector">, Group,
   HelpText<"Enable stack protectors for some functions vulnerable to stack smashing. "
-   "This uses a loose heuristic which considers functions vulnerable "
-   "if they contain a char (or 8bit integer) array or constant sized calls to "
-   "alloca, which are of greater size than ssp-buffer-size (default: 8 bytes). "
-   "All variable sized calls to alloca are considered vulnerable">;
+   "This uses a loose heuristic which considers functions vulnerable if they "
+   "contain a char (or 8bit integer) array or constant sized calls to alloca "
+   ", which are of greater size than ssp-buffer-size (default: 8 bytes). All "
+   "variable sized calls to alloca are considered vulnerable. A function with"
+   "a stack protector has a guard value added to the stack frame that is "
+   "checked on function exit. The guard value must be positioned in the "
+   "stack frame such that a buffer overflow from a 

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

[Peter Smith via Phabricator via cfe-commits]

psmith added a comment.

In D85239#2194604 , @MaskRay wrote:

> This file is auto generated. The real documentation should go to 
> `clang/include/clang/Driver/Options.td`

Thanks for pointing that out! I regenerated the ClangCommandLineReference.rst 
from Options.td but it looks like I'm not the only one that missed the comment 
on the top line. There are 27 differences in the file including some that are 
not in Options.td. I'm loathe to lose work by regenerating the whole file so 
I've included just the diff created by the change to Options.td. At least if 
the file is synched up again then we'll keep the edits.

Assuming this is still OK I'll commit tomorrow.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85239/new/

https://reviews.llvm.org/D85239

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

[Peter Smith via Phabricator via cfe-commits]

psmith created this revision.
psmith added reviewers: jfb, Shayne, emaste, kristof.beyls, mattdr, ojhunt, 
probinson, reames, serge-sans-paille, dim.
Herald added a subscriber: dexonsmith.
psmith requested review of this revision.

The Clang -fstack-protector documentation mentions what functions are 
considered vulnerable but does not mention the details of the
implementation such as the use of a global variable for the guard value. This 
brings the documentation more in line with the GCC
documentation at: 
https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html and gives 
someone using the option more idea about what is protected.

This partly addresses https://bugs.llvm.org/show_bug.cgi?id=42764


https://reviews.llvm.org/D85239

Files:
  clang/docs/ClangCommandLineReference.rst


Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -2136,7 +2136,7 @@
 
 .. option:: -fstack-protector, -fno-stack-protector
 
-Enable stack protectors for some functions vulnerable to stack smashing. This 
uses a loose heuristic which considers functions vulnerable if they contain a 
char (or 8bit integer) array or constant sized calls to alloca, which are of 
greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls 
to alloca are considered vulnerable
+Enable stack protectors for some functions vulnerable to stack smashing. This 
uses a loose heuristic which considers functions vulnerable if they contain a 
char (or 8bit integer) array or constant sized calls to alloca, which are of 
greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls 
to alloca are considered vulnerable. A function with a stack protector has a 
guard value added to the stack frame that is checked on function exit. The 
guard value must be positioned in the stack frame such that a buffer overflow 
from a vulnerable variable will overwrite to the guard value before overwriting 
the function's return address. The reference stack guard value is stored in a 
global variable.
 
 .. option:: -fstack-protector-all
 


Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -2136,7 +2136,7 @@
 
 .. option:: -fstack-protector, -fno-stack-protector
 
-Enable stack protectors for some functions vulnerable to stack smashing. This uses a loose heuristic which considers functions vulnerable if they contain a char (or 8bit integer) array or constant sized calls to alloca, which are of greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls to alloca are considered vulnerable
+Enable stack protectors for some functions vulnerable to stack smashing. This uses a loose heuristic which considers functions vulnerable if they contain a char (or 8bit integer) array or constant sized calls to alloca, which are of greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls to alloca are considered vulnerable. A function with a stack protector has a guard value added to the stack frame that is checked on function exit. The guard value must be positioned in the stack frame such that a buffer overflow from a vulnerable variable will overwrite to the guard value before overwriting the function's return address. The reference stack guard value is stored in a global variable.
 
 .. option:: -fstack-protector-all
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80828: [Clang][A32/T32][Linux] -O1 implies -fomit-frame-pointer

[Peter Smith via Phabricator via cfe-commits]

psmith accepted this revision.
psmith added a comment.

LGTM from an Arm person now that the Android changes have been made.




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:22
 #include "InputInfo.h"
+#include "MSP430.h"
 #include "PS4CPU.h"

nickdesaulniers wrote:
> Note to reviewers: the linter really wanted to touch this header inclusion 
> list, since I add "llvm/Support/Compiler.h" below.  Hopefully it's ok to just 
> include with this patch?
One possible solution here is to commit the reordering separately using an 
[NFC] refactoring commit, no separate review required, then commit the rest of 
the patch after that. Anyone doing a source code search will only pick up 
significant changes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80828/new/

https://reviews.llvm.org/D80828



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80828: [Clang][A32/T32][Linux] -O2 implies -fomit-frame-pointer

[Peter Smith via Phabricator via cfe-commits]

psmith added a comment.

FWIW I'm happy for Clang to match GCC behaviour for Linux (non-Android) targets 
with respect to the frame pointer. Outside of Android I would expect most 
developers of linux applications to build with both GCC and clang so they 
should already have the -fno-omit-frame-pointer if they are using it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80828/new/

https://reviews.llvm.org/D80828



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78565: [clang][doc] Clang ARM CPU command line argument reference

[Peter Smith via Phabricator via cfe-commits]

psmith added a comment.

One question I can't answer and I think would need wider review, is whether 
this is type of material (common options for specific CPUs) is suited for the 
Clang Documentation or whether it would be better hosted by Arm itself, for 
example on developer.arm.com? I think that a more reference like Arm CPU 
options like https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html would work 
better in the Clang documentation.

I think the material is useful, especially for people new to these CPUs.




Comment at: clang/docs/ClangARMCPUsCLI.rst:27
+
+.. option:: --target=arm-arm-none-eabi -mcpu=cortex-m33 -mfpu=fp-armv8-sp-d16 
-mfloat-abi=hard -mthumb
+

I think you mean arm-arm-none-eabi for an upstream triple selecting the 
bare-metal driver?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78565/new/

https://reviews.llvm.org/D78565



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78511: [Driver][doc] Document option -mtune as a no-op. NFC.

[Peter Smith via Phabricator via cfe-commits]

psmith accepted this revision.
psmith added a comment.
This revision is now accepted and ready to land.

That wording looks good to me. I've accepted the change, but worth waiting a 
day or so to see if there are any objections or suggestions.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78511/new/

https://reviews.llvm.org/D78511



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73904: [clang] stop baremetal driver to append .a to lib

[Peter Smith via Phabricator via cfe-commits]

psmith accepted this revision.
psmith added a comment.
This revision is now accepted and ready to land.

Thanks for the update, looks good to me. The BareMetal driver tests are better 
than the location I suggested.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73904/new/

https://reviews.llvm.org/D73904



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73904: [clang] stop baremetal driver to append .a to lib

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I think this is the right thing to do. According to 
https://sourceware.org/binutils/docs/ld/Options.html#Options

  Add the archive or object file specified by namespec to the list of files to 
link. This option may be used any number of times. If namespec is of the form 
:filename, ld will search the library path for a file called filename, 
otherwise it will search the library path for a file called libnamespec.a.

An argument could be made that LLD could be a bit cleverer and only add the .a 
when it doesn't exist, although that would fail in the contrived case that 
someone had explicitly named their library lib name.a.a however it would be 
good to fix this in clang so that older versions of LLD will work.

Please could you add a test to clang/test/Driver/arm-compiler-rt.c for the 
arm-none-eabi target.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73904/new/

https://reviews.llvm.org/D73904



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73865: [CodeGenModule] Assume dso_local for -fpic -fno-semantic-interposition

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

If I've understood correctly this would make LLVM more aggressive for PIC 
relocation models, but perhaps more honest in an inter procedural optimisation 
context? The code changes look fine to me, I'm wondering if we've discussed 
this widely enough with the community to work out how to proceed here. For 
example do we have plan to test -fno-semantic-interposition well enough for it 
to be relied on. The consensus may already exist, and I don't know enough about 
it though.




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:845
 
   // If this is not an executable, don't assume anything is local.
   const auto  = CGM.getCodeGenOpts();

I think that this comment needs updating to explain the effect of 
SemanticInterposition, and maybe the clang default.



Comment at: clang/test/CodeGen/aapcs-align.cpp:21
 }
-// CHECK: define void @g0
+// CHECK: define dso_local void @g0
 // CHECK: call void @f0(i32 1, [2 x i32] [i32 6, i32 7]

I initially thought a triple of arm-none-none-eabi (bare-metal for embedded 
systems) would have a relocation model of static and hence should have already 
been dso_local. Thinking about it in more detail the clang-driver will usually 
pass the relocation model to cc1 so by default the driver is assuming PIC. May 
be worth pointing that out in your description.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73865/new/

https://reviews.llvm.org/D73865



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In D7#1836703 , @MaskRay wrote:

> In D7#1836669 , @hans wrote:
>
> > In D7#1836643 , @peter.smith 
> > wrote:
> >
> > > >> If the patchable functions is intended for clang-10 we'll need to make 
> > > >> sure any fix is merged to clang-10.
> > > > 
> > > > This commit was made before release/10.x branch. Maybe the easiest 
> > > > thing is to revert the driver change in release/10.x (CC @hans), before 
> > > > we had a better understanding of the problem.
> > > >  (Eventually I think the Linux kernel should have a better configure 
> > > > time test than a simple `whether the compiler accepts 
> > > > -fpatchable-function-entry=2,0?`)
> > > > 
> > > > @peter.smith @nickdesaulniers What do you think?
> > >
> > > Revert on the 10.0 release sounds reasonable to me. That would prevent 
> > > the kernel from enabling the option and would prevent the build failure.
> >
> >
> > But if trunk is broken, shouldn't it be reverted there first, and then we 
> > can merge the revert to 10.x (and then trunk can be fixed eventually)?
>
>
> I don't think the commit is to be blamed. The availability of the driver 
> option -fpatchable-function-entry= enables `CONFIG_DYNAMIC_FTRACE_WITH_REGS` 
> and `CONFIG_LIVEPATCH`, which were not tested before. There could be other 
> issues in the code path.
>
>   % rg fpatchable-function-entry
>   arch/arm64/Kconfig
>   147:if $(cc-option,-fpatchable-function-entry=2)
>  
>   arch/arm64/Makefile
>   100:  CC_FLAGS_FTRACE := -fpatchable-function-entry=2
>
>
> If we can explicitly disable the options in the CI, that will be very nice.


It definitely won't be this commit, the assert fail looks like it is from the 
patchable-function attribute pass. The CI just runs allyesconfig and 
allmodconfig and tells us if there are any new regressions, it won't report 
this one again so it is unlikely to be worth masking it out.

Assert failure:

  clang-10: /work/llvm-project/llvm/include/llvm/ADT/ilist_iterator.h:139: 
llvm::ilist_iterator::reference 
llvm::ilist_iterator, false, false>::operator*() const [OptionsT = 
llvm::ilist_detail::node_options, 
IsReverse = false, IsConst = false]: Assertion `!NodePtr->isKnownSentinel()' 
failed.
  Stack dump:
  0.Program arguments: /work/llvm-project/build/buildclang/bin/clang-10 
-cc1 -triple aarch64-unknown-linux-gnu -S -disable-free -main-file-name 
slab_common.c -mrelocation-model static -mthread-model posix 
-fno-delete-null-pointer-checks -mllvm -warn-stack-size=2048 
-mframe-pointer=non-leaf -relaxed-aliasing -mdisable-tail-calls -fmath-errno 
-fno-rounding-math -masm-verbose -no-integrated-as -mconstructor-aliases 
-target-cpu generic -target-feature -fp-armv8 -target-feature -crypto 
-target-feature -neon -target-feature -sha2 -target-feature -aes -target-abi 
aapcs -mllvm -aarch64-enable-global-merge=false 
-fallow-half-arguments-and-returns -dwarf-column-info -fno-split-dwarf-inlining 
-debugger-tuning=gdb -nostdsysteminc -nobuiltininc -resource-dir 
/work/llvm-project/build/buildclang/lib/clang/10.0.0 -dependency-file 
mm/.slab_common.o.d -MT mm/slab_common.o -sys-header-deps -isystem 
/work/clangmaster/lib/clang/10.0.0/include -include ./include/linux/kconfig.h 
-include ./include/linux/compiler_types.h -I ./arch/arm64/include -I 
./arch/arm64/include/generated -I ./include -I ./arch/arm64/include/uapi -I 
./arch/arm64/include/generated/uapi -I ./include/uapi -I 
./include/generated/uapi -D __KERNEL__ -D CC_USING_PATCHABLE_FUNCTION_ENTRY -D 
KASAN_SHADOW_SCALE_SHIFT=3 -D CONFIG_AS_LSE=1 -D CONFIG_CC_HAS_K_CONSTRAINT=1 
-D KASAN_SHADOW_SCALE_SHIFT=3 -D KBUILD_BASENAME="slab_common" -D 
KBUILD_MODNAME="slab_common" -fmacro-prefix-map=./= -O2 -Wall -Wundef 
-Werror=strict-prototypes -Wno-trigraphs -Werror=implicit-function-declaration 
-Werror=implicit-int -Wno-format-security -Werror=unknown-warning-option 
-Wno-address-of-packed-member -Wno-format-invalid-specifier -Wno-gnu 
-Wno-tautological-compare -Wno-unused-const-variable 
-Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Werror=date-time 
-Werror=incompatible-pointer-types -Wno-initializer-overrides -Wno-format 
-Wno-sign-compare -Wno-format-zero-length -std=gnu89 -fno-dwarf-directory-asm 
-fdebug-compilation-dir /work/linux/linux -ferror-limit 19 -fmessage-length 0 
-fpatchable-function-entry=2 -fwrapv -stack-protector 2 
-ftrivial-auto-var-init=pattern -fno-builtin -fno-signed-char 
-fwchar-type=short -fno-signed-wchar -fgnuc-version=4.2.1 -fobjc-runtime=gcc 
-fno-common -fdiagnostics-show-option -fcolor-diagnostics -vectorize-loops 
-vectorize-slp -o /tmp/slab_common-7e60cc.s -x c mm/slab_common.c 
  1. parser at end of file
  2.Code generation
  3.Running pass 'Function Pass Manager' on module 'mm/slab_common.c'.
  4.Running pass 'Implement the 'patchable-function' attribute' on 

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

>> @peter.smith @nickdesaulniers What do you think?
> 
> Revert on the 10.0 release sounds reasonable to me. That would prevent the 
> kernel from enabling the option and would prevent the build failure.

I should have been clearer, apologies; we're not blocked by the assertion 
failures, the CI won't halt and should pick up other build failures.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D7/new/

https://reviews.llvm.org/D7



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

>> If the patchable functions is intended for clang-10 we'll need to make sure 
>> any fix is merged to clang-10.
> 
> This commit was made before release/10.x branch. Maybe the easiest thing is 
> to revert the driver change in release/10.x (CC @hans), before we had a 
> better understanding of the problem.
>  (Eventually I think the Linux kernel should have a better configure time 
> test than a simple `whether the compiler accepts 
> -fpatchable-function-entry=2,0?`)
> 
> @peter.smith @nickdesaulniers What do you think?

Revert on the 10.0 release sounds reasonable to me. That would prevent the 
kernel from enabling the option and would prevent the build failure.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D7/new/

https://reviews.llvm.org/D7



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Although this particular commit will not be at fault, it is the option that 
enables the feature which is the earliest I can bisect the fault to. There are 
3 files in linux that assert fail on the Implement the 'patchable-function 
attribute'. The files in question are kasan/quarantine.c, mm/slab_common.c and 
mm/slub.c .

I reproduced with

  make CC=/path/to/clang/install/clang ARCH=arm64 
CROSS_COMPILE=aarch64-linux-gnu- HOSTGCC=gcc allmodconfig

You can get the log files for the build, which is from clang-10
https://git.linaro.org/toolchain/ci/base-artifacts.git/log/?h=linaro-local/ci/tcwg_kernel/llvm-release-aarch64-mainline-allmodconfig
 (4: update: llvm-linux: 19676)

If the patchable functions is intended for clang-10 we'll need to make sure any 
fix is merged to clang-10.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D7/new/

https://reviews.llvm.org/D7



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72959: Relative VTables ABI on Fuchsia

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In D72959#1835368 , @rjmccall wrote:

> There's two independently-useful things here for the relocation, right?  
> First, it's useful to be able to express a relocation to a symbol that has 
> the semantics of a particular function but doesn't necessarily have its 
> address, and that concept could be used across many "physical" relocations; 
> and second, it's potentially useful to have a shifted-by-two relative address 
> relocation, at least on AArch64 where instructions (and v-table entries under 
> this ABI) are always four-byte-aligned anyway.  Is it possible to separate 
> these concerns in ELF, e.g. by having a "symbol" that can be the target of 
> any other relocation but which actually represents a function of unspecified 
> address with the semantics of another function?


It would be possible to design relocations like that, but I think it would be 
difficult to fit into existing multi-target designs, which assume that the 
relocation code alone encodes all the actions a linker needs to take 
(smart-format, dumb-linker). My understanding of the reasons behind this are:

- The linker can have millions of relocations to resolve and  having all the 
actions explicit in the code simplifies its job.
- There is a large space of available relocation codes, partitioned per target, 
but a much more limited availability of target specific symbol types and flags.
- The concerns can be separated at implementation time, for example the symbol 
lookup, encoding and calculation stages are independent and shared between the 
codes.

It is possible that there is a better trade-off in complexity, but anything 
radically different might need quite a bit of work to fit into an existing 
linker. Hope I've understood you correctly and apologies if the above isn't 
relevant.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72959/new/

https://reviews.llvm.org/D72959



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72959: Relative VTables ABI on Fuchsia

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In D72959#1832011 , @pcc wrote:

> > On Aarch64, right now only CALL26 and JUMP26 instructions generate PLT 
> > relocations, so we manifest them with stubs that are just jumps to the 
> > original function.
>
> I think it would be worth considering defining a new relocation type for 
> this. I think we should make the new reloc type represent the relative 
> address shifted right by 2, which would allow it to be used freely in the 
> aarch64 small code model (which permits programs up to 4GB), but would 
> require the target to be 4-byte aligned, which seems like a reasonable 
> restriction. On aarch64, decoding this would not require any additional 
> instructions either (we can fold the lsl into the add). We could use the same 
> technique for a new GOTPCREL relocation to be used for the RTTI component. 
> @peter.smith what do you think?


The idea of a new relocation type has come up before, as I recall it was 
something like the equivalent of R_X86_PLT32

  | R_X86_64_PLT32 | 4 | word32 | L + A - P |
  Where L is defined as: L Represents the place (section offset or address) of 
the Procedure Linkage Table entry for a symbol.

For Fuchsia there are two options:
1.) Ask for an ABI relocation type to be defined. I've raised an ABI issue with 
Arm, for it to progress I think it needs a "yes we really would like one, here 
is the definition we want to use, this is the use case and it could be used 
outside of Fuchsia at some point." For example I can see position independent 
C++ being useful in bare-metal embedded C++. The external facing email for 
feedback on the abi is arm.e...@arm.com (address is on the first page of the 
doc below)
2.) There are two ranges of relocation types reserved for private and platform 
relocations: 
https://developer.arm.com/docs/ihi0056/f/elf-for-the-arm-64-bit-architecture-aarch64-abi-2019q2-documentation

  Private and platform-specific relocations
  Private relocations for vendor experiments:
  
  0xE000 to 0xEFFF for ELF64
  0xE0 to 0xEF for ELF32
  Platform ABI defined relocations:
  
  0xF000 to 0x for ELF64
  0xF0 to 0xFF for ELF32
  Platform ABI relocations can only be interpreted when the EI_OSABI field is 
set to indicate the Platform ABI governing the definition.
  
  All of the above codes will not be assigned by any future version of this 
standard.

If this could be generally useful outside of Fuchsia, then an official 
relocation would be preferable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72959/new/

https://reviews.llvm.org/D72959



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72897: List implicit operator== after implicit destructors in a vtable.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

It is also failing on the other 32-bit arm bots, example 
http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/13052/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Avirtual-compare.cpp
Looking at the failure

  // CHECK-SAME: @_ZThn8_N1AD1Ev 

and the output on an Arm machine

  $_ZThn4_N1AD1Ev = comdat any
  
  $_ZThn4_N1AD0Ev = comdat any
  
  $_ZThn4_N1AaSERKS_ = comdat any

It might be a 32-bit/64-bit specific expectation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72897/new/

https://reviews.llvm.org/D72897



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71688: [AArch64] Add -mtls-size option for ELF targets

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Committed as 10c11e4e2d05cf0e8f8251f50d84ce77eb1e9b8d 
 , I've 
used the new attribution process https://llvm.org/docs/DeveloperPolicy.html so 
you should show up as the author of the patch in the commit log. I'll comment 
here if there are any problems with buildbots.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71688/new/

https://reviews.llvm.org/D71688



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71688: [AArch64] Add -mtls-size option for ELF targets

[Peter Smith via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG10c11e4e2d05: This option allows selecting the TLS size in 
the local exec TLS model, which is… (authored by kawashima-fj, committed by 
peter.smith).
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71688/new/

https://reviews.llvm.org/D71688

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/tls-size.c
  llvm/include/llvm/CodeGen/CommandFlags.inc
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
  llvm/lib/Target/AArch64/AArch64ISelLowering.h
  llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
  llvm/test/CodeGen/AArch64/arm64-tls-execs.ll
  llvm/test/CodeGen/AArch64/arm64-tls-initial-exec.ll
  llvm/test/CodeGen/AArch64/arm64-tls-local-exec.ll

Index: llvm/test/CodeGen/AArch64/arm64-tls-local-exec.ll
===
--- /dev/null
+++ llvm/test/CodeGen/AArch64/arm64-tls-local-exec.ll
@@ -0,0 +1,106 @@
+; Test each TLS size option
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -tls-size=12 < %s | FileCheck %s --check-prefix=CHECK-12
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -tls-size=12 | llvm-objdump -r - | FileCheck --check-prefix=CHECK-12-RELOC %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -code-model=tiny -tls-size=24 < %s | FileCheck %s --check-prefix=CHECK-24
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -code-model=tiny -tls-size=24 | llvm-objdump -r - | FileCheck --check-prefix=CHECK-24-RELOC %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -code-model=small -tls-size=32 < %s | FileCheck %s --check-prefix=CHECK-32
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -code-model=small -tls-size=32 | llvm-objdump -r - | FileCheck --check-prefix=CHECK-32-RELOC %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -code-model=large -tls-size=48 < %s | FileCheck %s --check-prefix=CHECK-48
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -code-model=large -tls-size=48 | llvm-objdump -r - | FileCheck --check-prefix=CHECK-48-RELOC %s
+;
+; Test the maximum TLS size for each code model (fallback to a smaller size from the specified size)
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -tls-size=32 < %s | FileCheck %s --check-prefix=CHECK-32
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -tls-size=32 | llvm-objdump -r - | FileCheck --check-prefix=CHECK-32-RELOC %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -code-model=tiny -tls-size=32 < %s | FileCheck %s --check-prefix=CHECK-24
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -code-model=tiny -tls-size=32 | llvm-objdump -r - | FileCheck --check-prefix=CHECK-24-RELOC %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -code-model=small -tls-size=48 < %s | FileCheck %s --check-prefix=CHECK-32
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -code-model=small -tls-size=48 | llvm-objdump -r - | FileCheck --check-prefix=CHECK-32-RELOC %s
+;
+; Test the default TLS size for each code model
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding < %s | FileCheck --check-prefix=CHECK-24 %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s | llvm-objdump -r - | FileCheck --check-prefix=CHECK-24-RELOC %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -code-model=tiny < %s | FileCheck %s --check-prefix=CHECK-24
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -code-model=tiny | llvm-objdump -r - | FileCheck --check-prefix=CHECK-24-RELOC %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -code-model=small < %s | FileCheck %s --check-prefix=CHECK-24
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -code-model=small | llvm-objdump -r - | FileCheck --check-prefix=CHECK-24-RELOC %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -code-model=large < %s | FileCheck %s --check-prefix=CHECK-24
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -code-model=large | llvm-objdump -r - | FileCheck --check-prefix=CHECK-24-RELOC %s
+
+@local_exec_var = thread_local(localexec) global i32 0
+
+define i32 @test_local_exec() {
+; CHECK-LABEL: test_local_exec:
+  %val = load i32, i32* @local_exec_var
+
+; CHECK-12: mrs x[[R1:[0-9]+]], TPIDR_EL0
+; CHECK-12: add x[[R2:[0-9]+]], x[[R1]], :tprel_lo12:local_exec_var
+; CHECK-12: ldr w0, 

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

Thanks for the update. That looks good to me. Will be good to wait for a day 
before committing to give US timezone a chance to object.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67608/new/

https://reviews.llvm.org/D67608



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

After retesting on a failing example and experimenting a bit, I think that we 
should only preserve +crypto with -fno-integrated-as. It would also be good to 
mention D66018  and maybe add the original 
author as a reviewer.




Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:499
 << "crypto" << 
llvm::ARM::getArchName(llvm::ARM::parseArch(ArchSuffix));
-  Features.push_back("-crypto");
+  //-mfpu=crypto-neon-fp-armv8 does allow +sha2 / +aes / +crypto features,
+  // even if compiling for an cpu armv7a, if explicitly defined by the 
user,

Looking into this in more detail I think the comment can be made more specific. 
It seems like the main reason to keep +crypto is that when the 
non-integrated-assembler is used, then we get the directive:
```
.fpu crypto-neon-fp-armv8
```
In arm-none-eabi-as the assembler will support crypto instructions. Clang will 
not as any use of crypto instructions will fail due lack of v8 support.

```
With -fno-integrated-as -mfpu=crypto-neon-fp-armv8 some assemblers such as the 
GNU assembler will permit the use of crypto instructions as the fpu will 
override the architecture. We keep the crypto feature in this case to preserve 
compatibility. In all other cases we remove the crypto feature. 
```



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:502
+  // so do not deactivate them.
+  if ((llvm::find(Features, "+fp-armv8") == Features.end()) ||
+  (llvm::find(Features, "+neon") == Features.end()))

I think we should only do this for -fno-integrated-as as the integrated 
assembler will fail if give a crypto instruction even with this change. With 
the integrated assembler we get:

```
error: instruction requires: armv8
aese.8 q8, q9
```



Comment at: clang/test/Driver/arm-features.c:84
+// Check +crypto does not affect -march=armv7a -mfpu=crypto-neon-fp-armv8, but 
it does warn that +crypto has no effect
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a 
-mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a+aes 
-mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASAES %s

arm-arm-none-eabi is a vendor specific triple? Better to use arm-none-eabi 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67608/new/

https://reviews.llvm.org/D67608



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65000: [ARM] Set default alignment to 64bits

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

I've not seen any objections so I've approved LGTM.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65000/new/

https://reviews.llvm.org/D65000



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65000: [ARM] Set default alignment to 64bits

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

test case missing A8 aside this looks ok to me. Would like to see if there are 
any comments from the Pacific time zone.




Comment at: test/CodeGen/ARM/exception-alignment.cpp:8
+// A16-NEXT: store <2 x i64> , <2 x i64>* [[BC]], align 16
+#include 
+

Are you missing the A8 case? presumably:
```
store <2 x i64> , <2 x i64>* [[BC]], align 8
```



Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65000/new/

https://reviews.llvm.org/D65000



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65000: [ARM] Set default alignment to 64bits

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Thanks for the update. Will be worth adding some reviewers from Apple to see if 
this change should be IsAAPCS only. I've no more further comments myself 
besides a small nit on style.




Comment at: lib/Basic/Targets/ARM.cpp:331
+   // but Android ABI uses 128-bit alignment as default
+  DefaultAlignForAttributeAligned =
+   (Triple.getEnvironment() == llvm::Triple::Android)?128:64;

Nit: suggest running clang-format over the conditional expression I think that 
spaces are needed around ? and :

The previous bit of code used
```
if (IsAAPCS && (Triple.getEnvironment() != llvm::Triple::Android))
MaxVectorAlign = 64;
```
The IsAAPCS could be important here as on Macho targets IsAAPCS may not be 
true, see calls to setABI in ARMTargetInfo::ARMTargetInfo.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65000/new/

https://reviews.llvm.org/D65000



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65000: [ARM] Set default alignment to 64bits

[Peter Smith via Phabricator via cfe-commits]

peter.smith added reviewers: srhines, danalbert.
peter.smith added a comment.

I think that this may not apply for Android as AFAIK their ABI still requires 
128-bit alignment in some cases. Adding some more reviewers from Android.




Comment at: lib/Basic/Targets/ARM.cpp:311
 
   // Maximum alignment for ARM NEON data types should be 64-bits (AAPCS)
   if (IsAAPCS && (Triple.getEnvironment() != llvm::Triple::Android))

I think that Android can require a higher alignment in some cases (See below). 
I think that this was explained in D33205


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65000/new/

https://reviews.llvm.org/D65000



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64415: Consistent types and naming for AArch64 target features (NFC)

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

FWIW I think this looks reasonable. The ARM equivalent uses bitfields such as

  unsigned CRC : 1;
  unsigned Crypto : 1;
  unsigned DSP : 1;
  unsigned Unaligned : 1;
  unsigned DotProd : 1;

Which would make more sense than using unsigned for each individual field. 
Several other targets that I looked at used bool and the Has 
convention so I think this brings AArch64 more in line with non ARM Targets at 
the cost of losing a little coherence with ARM where the name Crypto remains. I 
don't think that this is particularly important as the two have already started 
to diverge with HasDotProd.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64415/new/

https://reviews.llvm.org/D64415



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60472: [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In D60472#1461336 , @manojgupta wrote:

> The motivation for this change is to make "crypto" setting an additive option 
> e.g. like "-mavx" used in many media packages.  Some packages in Chrome want 
> to enable crypto conditionally for a few files to allow crypto feature to be 
> used based on runtime cpu detection. They set "-march=armv8+crypto" flag but 
> it gets overridden by the global "-march=armv8a" flag set by the build system 
> in Chrome OS because the target cpu does not support crypto causing 
> compile-time errors. 
>  Ability to specify "-mcrypto"  standalone makes it an additive option and 
> ensures that it it is not lost. i.e. this will help in decoupling  "-mcrypto" 
> from "-march" so that they could be set independently. The current additive 
> alternate is  '-Xclang -target-feature -Xclang "+crypto" ' which is ugly.


Is that not a limitation of the build system? I'd expect a package to be able 
to locally override a global default rather than vice-versa. Although crypto 
might be the area of concern here and there may be a conveniently named option 
for PPC, where does this stop? Crypto is not the only architectural extension, 
for example see https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html . To 
maintain a consistent interface we'd need command line options for all the 
extensions. May I encourage you to reply to the RFC on command line options 
that I mentioned earlier if it doesn't work for you? I think the extensions 
need to be considered as a whole and not just individually.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60472/new/

https://reviews.llvm.org/D60472



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60472: [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I'm not in favour of adding AArch64 support to -mcrypto -mnocrypto for a few 
reasons:

- Arm are trying to keep the options for controlling target features as 
consistent as possible with GCC and this option isn't supported in GCC 
(https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html)
- There is http://lists.llvm.org/pipermail/llvm-dev/2018-September/126346.html 
which is Arm's proposal for how target options can be better supported in 
Clang. I think that supporting -mcrypto as well would add more complexity as 
there are interactions between the options.
- Arm 32-bit also supports crypto so this would need to be added to Arm as well 
for consistency.

Can you expand on why you need -m[no]crypto?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60472/new/

https://reviews.llvm.org/D60472



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58320: [Darwin] Introduce a new flag, -fapple-link-rtlib that forces linking of the builtins library.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I've no objections to adding the command line as a Darwin only option. 
Implementation looks fine to me although I've not got any prior experience with 
Darwin.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58320/new/

https://reviews.llvm.org/D58320



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58314: [Driver] Sync ARM behavior between clang-as and gas.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Thanks for the update. To summarise I'd like to keep the integrated and 
non-integrated assembler in synch for Linux like Android, even at the risk of 
adding an fpu when it isn't needed. I think that given how difficult it is to 
not use NEON, I think the mfloat-abi=soft guard you've put on will be 
sufficient. By the way, I'm hoping we aren't talking past each other with the 
default for armv7-a. I've tried to put as much of what I understand in the 
comments and I hope the answers make sense, or you'll be able to correct me 
where I'm wrong. If the timezone delayed comments aren't working well it may be 
worth finding me on IRC, I usually leave the office about 6:30pm but I can 
easily arrange a time and log on later if you wanted to discuss.

I also spotted something else on line 251 of  ARM.cpp with respect to Android 
that might be worth looking at. I've left a comment although it isn't related 
to this patch.




Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:277
+  }
+  return "";
+}

danalbert wrote:
> peter.smith wrote:
> > I'm a bit worried that we've changed the default behaviour for gnueabi[hf] 
> > targets here. 
> > For example with:
> > ```
> > .text
> > vmov.32 d13[1], r6 ; Needs VFPv2
> > vadd.i32 d0, d0, d0 ; Needs Neon
> > ```
> > I get with --target=armv7a-linux (no environment, -mfloat-abi will disable 
> > floating point, and neon)
> > ```
> > clang: warning: unknown platform, assuming -mfloat-abi=soft
> > neon.s:2:9: error: instruction requires: VFP2
> > vmov.32 d13[1],r6
> > ^
> > neon.s:3:9: error: instruction requires: NEON
> > vadd.i32 d0, d0, d0
> > ^
> > ```
> > With the target=armv7a-linux-gnueabi armv7a-linux-gnueabihf or explicitly 
> > adding -mfloat-abi=softfp the integrated assembler will happily assemble it.
> > GNU needs -mfpu=neon to assemble the file:
> > 
> > ```
> > arm-linux-gnueabihf-as -march=armv7-a neon.s 
> > neon.s: Assembler messages:
> > neon.s:2: Error: selected processor does not support ARM mode `vmov.32 
> > d13[1],r6'
> > neon.s:3: Error: selected processor does not support ARM mode `vadd.i32 
> > d0,d0,d0'
> > ```
> > It is a similar story for armv8 and crypto.
> > 
> > I think we should have something like:
> > ```
> > if (Triple.isLinux() && getARMSubArchVersionNumber(Triple) >= 8)
> >return "crypto-neon-fp-armv8";
> > if (Triple.isAndroid() || Triple.isLinux() && 
> > getARMSubArchVersionNumber(Triple) >= 7)
> > return "neon";
> > return "";
> > ```
> I suppose it depends on which direction you want the behavior change to go. I 
> assumed those samples _shouldn't_ assemble since they're not enabling NEON. 
> The fact that the direct `arm-linux-gnueabihf-as` doesn't enable NEON by 
> default makes me assume that NEON is not an assumed feature of the gnueabihf 
> environment.
> 
> It's not up to me whether NEON is available by default for ARMv7 for 
> non-Android, but I do think that the behavior should be consistent regardless 
> of the assembler being used. Right now we have no FPU by default with the 
> integrated assembler and NEON by default with GAS. This change makes GAS have 
> the same behavior as the integrated assembler, since I assume that is the 
> better traveled path and afaict is the one that has had more effort put in to 
> it.
> 
> If that's not right, I can change this so that the integrated assembler 
> _also_ gets FPU features that are not necessarily available for the given 
> architecture, but I wanted to clarify that that is what you're asking for 
> first.
> I suppose it depends on which direction you want the behavior change to go. I 
> assumed those samples _shouldn't_ assemble since they're not enabling NEON. 
> The fact that the direct arm-linux-gnueabihf-as doesn't enable NEON by 
> default makes me assume that NEON is not an assumed feature of the gnueabihf 
> environment.

It is true that LLVM and GNU have unfortunately chosen a different default for 
armv7-a and NEON and armv8-a and crypo. I agree that NEON is not an assumed 
feature of either a gnueabi or gnueabihf GCC toolchain. My understanding is 
that GNU chose to not include any extensions in the base architecture and LLVM 
chose the most common configuration for the default.

>  Right now we have no FPU by default with the integrated assembler and NEON 
> by default with GAS. This change makes GAS have the same behavior as the 
> integrated assembler, since I assume that is the better traveled path and 
> afaict is the one that has had more effort put in to it.

Are you sure that we have no FPU by default with the integrated assembler for 
armv7-a and armv8-a? I've put an explanation in the next comment that explains 
how it gets set even when there is no +neon on the -cc1 or -cc1as command line. 

> If that's not right, I can change this so that the integrated assembler 
> _also_ gets FPU features that are not necessarily available for the given 
> 

[PATCH] D58429: [CodeGen] Enable the complex-math test for arm

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks for the fix. My apologies for missing that at the time, it looks 
like a cut and paste error on my part.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58429/new/

https://reviews.llvm.org/D58429



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58314: [Driver] Sync ARM behavior between clang-as and gas.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

My main concern is that this changes the default behaviour for 
arm-linux-gnueabi and arm-linux-gnueabihf targets. I've put some suggestions on 
what I think the behaviour should be.




Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:277
+  }
+  return "";
+}

I'm a bit worried that we've changed the default behaviour for gnueabi[hf] 
targets here. 
For example with:
```
.text
vmov.32 d13[1], r6 ; Needs VFPv2
vadd.i32 d0, d0, d0 ; Needs Neon
```
I get with --target=armv7a-linux (no environment, -mfloat-abi will disable 
floating point, and neon)
```
clang: warning: unknown platform, assuming -mfloat-abi=soft
neon.s:2:9: error: instruction requires: VFP2
vmov.32 d13[1],r6
^
neon.s:3:9: error: instruction requires: NEON
vadd.i32 d0, d0, d0
^
```
With the target=armv7a-linux-gnueabi armv7a-linux-gnueabihf or explicitly 
adding -mfloat-abi=softfp the integrated assembler will happily assemble it.
GNU needs -mfpu=neon to assemble the file:

```
arm-linux-gnueabihf-as -march=armv7-a neon.s 
neon.s: Assembler messages:
neon.s:2: Error: selected processor does not support ARM mode `vmov.32 
d13[1],r6'
neon.s:3: Error: selected processor does not support ARM mode `vadd.i32 
d0,d0,d0'
```
It is a similar story for armv8 and crypto.

I think we should have something like:
```
if (Triple.isLinux() && getARMSubArchVersionNumber(Triple) >= 8)
   return "crypto-neon-fp-armv8";
if (Triple.isAndroid() || Triple.isLinux() && 
getARMSubArchVersionNumber(Triple) >= 7)
return "neon";
return "";
```



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:389
+std::string DefaultFPU = getDefaultFPUName(Triple);
+if (DefaultFPU != "") {
+  if (!llvm::ARM::getFPUFeatures(llvm::ARM::parseFPU(DefaultFPU), 
Features))

I'm wondering whether you need this bit of code anymore? In D53121 there needed 
to be a switch between vfpv3-d16 and neon based on Android version. With 
--target=armv7a-linux-android or --target=arm-linux-android -march=armv7a or 
any v7a -mcpu applicable to Android then you'll get feature Neon by default and 
won't need to do this? We could then move getDefaultFPUName out of ARM.cpp



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:692
+}
+if (!hasMOrWaMArg(Args, options::OPT_mfpu_EQ, "-mfpu=")) {
+std::string DefaultFPU = arm::getDefaultFPUName(Triple);

I think we'd not want to do this for -mfloat-abi=soft as this disables the FPU 
in the integrated assembler. It seems like -mfloat-abi has no effect at all on 
the gnu assembler, it will happily assemble neon instructions with 
-mfloat-abi=soft -mfpu=neon.
 



Comment at: clang/test/Driver/linux-as.c:3
 //
 // RUN: %clang -target arm-linux -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \

the target arm-linux (effectively arm-linux-unknown) defaults to 
-mfloat-abi=soft which disables the FPU for the integrated assembler. While 
these test cases are not wrong, the number of v7a + linux targets without an 
FPU using entirely software floating point is likely to be very small. We 
should have some more that have arm-linux-gnueabi and arm-linux-gnueabihf.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58314/new/

https://reviews.llvm.org/D58314



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58320: [Darwin] Introduce a new flag, -flink-builtins-rt that forces linking of the builtins library.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

The implementation changes in the Darwin toolchain look fine to me, although 
with respect to the command line option I think Petr Hosek's message on cfe-dev 
is interesting:

> GCC implements -nolibc which could be used to achieve the same effect when 
> combined with -nostartfiles (and -nostdlib++ when compiling C++). I'd prefer 
> that approach not only because it improves compatibility with with GCC, but 
> also because it matches existing flag scheme which is subtractive rather than 
> additive (i.e. -nodefaultlibs, -nostdlib, -nostdlib++, -nostartfiles). Clang 
> already defines this flag but the only toolchain that currently supports it 
> is DragonFly.

Looking at https://gcc.gnu.org/onlinedocs/gcc/Link-Options.html (quoted here 
for convenience)

> -nostartfiles
>  Do not use the standard system startup files when linking. The standard 
> system libraries are used normally, unless -nostdlib, -nolibc, or 
> -nodefaultlibs is used.
> -nolibc
>  Do not use the C library or system libraries tightly coupled with it when 
> linking. Still link with the startup files, libgcc or toolchain provided 
> language support libraries such as libgnat, libgfortran or libstdc++ unless 
> options preventing their inclusion are used as well. This typically removes 
> -lc from the link command line, as well as system libraries that normally go 
> with it and become meaningless when absence of a C library is assumed, for 
> example -lpthread or -lm in some configurations. This is intended for 
> bare-board targets when there is indeed no C library available.

It does seem like these options accomplish what -flink_builtins_rt do with the 
added advantage of being more portable with gcc. If they don't work for you it 
will be worth double checking with Petr.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58320/new/

https://reviews.llvm.org/D58320



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56650: [lld] [ELF] Support customizing behavior on target triple

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I think we need to be very careful here. If I've understood this correctly then 
if this functionality is used for anything critical then a manually supplied 
target will be needed when doing cross-linking. For example my host LLD is 
x86_64, is just called ld.lld and will have an inferred x86_64 target triple. 
If someone customises the behaviour of LLD on the triple in a way that doesn't 
get caught by the test suite then we could get some strange breakages when 
doing cross-linking. I personally would prefer to see any option like this not 
try and auto-infer the target unless it can do it reliably and accurately from 
the input objects and I don't know if that is possible for all supported 
targets.

I think this might be better raised on lllvm-dev as I suspect that we need to 
give this wider visibility. I'm not totally opposed to this as I can see that 
--target has some advantages over adding extra emulations, or relying on the 
--target in the compiler driver but I think we need to be careful to define how 
this will interact with the emulation, and what the bounds of what we can 
customise with the option are?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56650/new/

https://reviews.llvm.org/D56650



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55029: set default max-page-size to 4KB in lld for Android Aarch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In D55029#1313120 , @ruiu wrote:

> LGTM. Please commit.
>
> Peter, I wonder if you are fine with the default 64KiB page size with lld, 
> especially given that lld always round up the text segment size to the 
> maximum page size on disk and fill the padding with trap instructions. On 
> average, that should increase the executable size by 32 KiB compared to other 
> linkers. I don't think that size is necessarily bad, because we are doing 
> that for a security purpose, but I wonder if people are OK with that.


I think the default is fine at 64KiB . Going back to 4KiB risks breaking 
programs that currently use default options on platforms that have chosen 64KiB 
which I think is a step too far. So far the concern about ELF file size has 
come from Android, where we have a separate target in clang where it is fairly 
easy to pass a 4KiB page size by default. There is a chance that this may 
change in the future as more general AArch64 linux platforms start being 
deployed on devices with limited storage. I guess at that point we could 
consider implementing common-page-size if it were a problem to pass 4KiB page 
size.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55029/new/

https://reviews.llvm.org/D55029



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits