[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

For me long matching prefix makes more sense, but if the same prefix is used 
multiple times, the last option should win.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148975

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

The patch contains at least one user visible change that would be quite 
surprising: it is no longer possible to intentionally set a break point on 
`std::move`.

Thinking more about it, what about a slightly different implementation 
strategy? Provide a compiler built-in `__builtin_std_move`. If it is present, 
libc++ can alias it into `namespace std` using regular C++. Much of the 
name-matching and argument checking in various places disappears. The check to 
skip template instantiation can be done the same way. Over-all, it should be 
much less intrusive with the same performance benefits for an built-in-aware 
STL. It completely side-steps the question of ignoring the actual 
implementation `of std::move` in the source, because there is none.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

As is, I think this conflicts with `-ffreestanding` assumptions or at the very 
least the spirit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D122077: [InstCombine] Fold (ctpop(X) == 1) | (X == 0) into ctpop(X) < 2

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Why is this fold preferable to `(X & (X-1)) == 0`? At least on all 
architectures without native population count, the binary-and based test is 
preferable and it might even be better with it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122077

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

[PATCH] D121512: [Support] Change zlib::compress to return void

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121512

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

[PATCH] D119655: [Driver][NetBSD] -r: imply -nostdlib like GCC

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Well, it doesn't work with GCC either, that's why I don't care much about this 
change. It just attempts to legalize a user bug (using a linker option directly 
as a compiler flag). But I don't care enough to object either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119655

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

[PATCH] D119655: [Driver][NetBSD] -r: imply -nostdlib like GCC

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

I'm ambivalent about this change. To me, it falls into the category of "stop 
passing random ld options to the compiler driver".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119655

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

[PATCH] D118021: [Driver] Use libatomic for 32-bit SPARC atomics support

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added inline comments.



Comment at: clang/lib/Driver/ToolChains/Solaris.cpp:135
 }
+// LLVM lacks atomics support on 32-bit SPARC, so forcibly link with
+// libatomic as a workaround.

This comment is misleading. It's not so much that LLVM doesn't support them, 
but that SPARCv8 doesn't have the necessary hardware support. The v8+ support 
is incomplete, which is a related problem though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118021

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

[PATCH] D116088: [compiler-rt] Implement ARM atomic operations for architectures without SMP support

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Correct me if I'm wrong, but I don't think this stubs are async signal safe nor 
will they work for preemptive multitasking systems?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116088

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

[PATCH] D116882: [docs] [clang] Small documentation change for compilation databases

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg accepted this revision.
joerg added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/docs/JSONCompilationDatabase.rst:33
+Clang has the ablity to generate compilation database fragments via
+the :option:`-MJ argument >`. You can contantinate those
+fragments together between ``[`` and ``]`` to create a compilation database.

I think you mean "concatenate", but otherwise LGTM? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116882

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

[PATCH] D113372: [Driver] Add CLANG_DEFAULT_PIE_ON_LINUX to emulate GCC --enable-default-pie

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Last update introduced a lot of unrelated changes? But the actual intended 
change seems fine now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113372

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

[PATCH] D113372: [Driver] Add CLANG_DEFAULT_PIE to emulate GCC --enable-default-pie

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added inline comments.



Comment at: clang/CMakeLists.txt:230
 
+option(CLANG_DEFAULT_PIE "Default to -fPIE and -pie (Linux only)" OFF)
+if(CLANG_DEFAULT_PIE)

This option should really be called something like CLANG_DEFAULT_PIE_ON_LINUX 
or so. It should be explicit from the name that it only effects that toolchain 
and not all the various toolchains that support PIE.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113372

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

[PATCH] D114564: Fix the use of -fno-approx-func along with -Ofast or -ffast-math

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2760
 case options::OPT_fno_honor_nans:   HonorNaNs = false;break;
 case options::OPT_fapprox_func: ApproxFunc = true;break;
 case options::OPT_fno_approx_func:  ApproxFunc = false;   break;

masoud.ataei wrote:
> masoud.ataei wrote:
> > andrew.w.kaylor wrote:
> > > Should this also imply "MathErrno = false"?
> > I don't think setting ApproxFunc to true should imply "MathErrno = false". 
> > 
> > Let say someone have a math library that compute approximate result for 
> > none special input/output but returns NaN, INF and errno correctly 
> > otherwise. That is actually can be fairly common, because performance in 
> > the none special cases are much more important that the special ones. So 
> > returning errno in the special outputs theoretically should not effect the 
> > performance on the main path. Therefore, I think compiler should not assume 
> > anything about MathErrno value based on ApproxFunc value.
> I am not sure what I was suggesting in my last comment is correct or not. Can 
> one of the more experienced reviewers confirm?
> The question is: Should "ApproxFunc=ture" implies "MathErrno=false"?
They seem pretty orthogonal to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114564

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

[PATCH] D112413: [X86] Add -mskip-rax-setup support to align with GCC

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added inline comments.



Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:99
+cl::desc("Skips setting up the RAX register when SSE is disabled and there 
"
+ "are no variable arguments passed in vector registers."),
+cl::Hidden);

The description needs some further clarification. It should likely be "can be 
passed" otherwise it begs the question of when the conditional statement is 
false.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112413

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

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

In D111863#3072023 , @housel wrote:

> It's also worth noting that FreeBSD's version of libgcc exception handling is 
> actually based on the libunwind code, with a local patch 
> 
>  that implements compatibility with libgcc `__register_frame` by changing it 
> to parse an entire `.eh_frame` section (in a slightly more ad hoc fashion 
> than this code). Having this new entry point in-tree would simplify the 
> FreeBSD local changes.

Are you mixing up of `__register_frame` and `__register_frame_info`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111863

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

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

In D111863#3071651 , @lhames wrote:

> In D111863#3071353 , @joerg wrote:
>
>> I would strongly prefer if ORCv2 doesn't depend on this. It essentially 
>> forces libunwind to parse the whole section just to find the delimiters of 
>> the FDEs. That's a lot of unnecessary work as JIT use normally allows 
>> registering functions individually.
>
> I don't follow this. Does libunwind provide some way to register FDEs without 
> parsing the FDE content? If so we can definitely use that, but we should 
> still process the whole section: ORC links objects (not functions), and we 
> should register every FDE for an object when it's linked in.
>
> It's also worth noting that ORC and MCJIT have always called 
> `__register_frame` on every frame, which seems like it should be at least as 
> much work as this.

`__register_frame` requires parsing the CIE header, but not the whole FDE 
program. E.g. that's the `findPCRange` logic. After that, the FDE is just added 
to the internal block list. Parsing a whole segment is more involved as it 
needs to look for the terminator of each block to find the next FDE.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111863

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

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

I would strongly prefer if ORCv2 doesn't depend on this. It essentially forces 
libunwind to parse the whole section just to find the delimiters of the FDEs. 
That's a lot of unnecessary work as JIT use normally allows registering 
functions individually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111863

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

[PATCH] D107825: [AIX] Define __HOS_AIX__ macro only for AIX target

[Joerg Sonnenberger via Phabricator via cfe-commits]

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

LGTM. Maybe include a small hint in the commit message that xlC never shipped 
cross-compiling support and the difference is therefore not observable anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107825

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

[PATCH] D107242: [AIX] Define __HOS_AIX__ macro

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

clang is fundamentally a cross-compiler only. I don't see any point for having 
host-specific branches in this case at all. Either the macro should be 
specified for the target all the time or not at all, but it should most 
definitely not depend on the host. That's actually breaking a number of 
existing use case for clang in subtle ways, e.g. partially preprocessed files 
(`-frewrite-includes`) should behave the same on any platform given the same 
command line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107242

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

[PATCH] D107242: [AIX] Define __HOS_AIX__ macro

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

I'm puzzled by this change. I don't think we have any case so far where the 
compiler behavior changes with the host OS and I don't think it should. What's 
the point / use case of this macro?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107242

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

[PATCH] D107304: [clangd][query-driver] Extract GCC version from the driver output

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

This discussion is becoming pointless. Clang is not a 1:1 replacement for later 
GCC versions. Easiest example is the support for `__builtin_apply` and friends.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107304

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

[PATCH] D107304: [clangd][query-driver] Extract GCC version from the driver output

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Yes, there are quite a number of small differences in builtin support and even 
certain macros that just invite even more trouble than this. This is IMO 
begging for hard to trace down errors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107304

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

[PATCH] D107304: [clangd][query-driver] Extract GCC version from the driver output

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

What do you mean with "GCC macros are correct"? Clang is *not* GCC 5 and not 
1:1 compatible with GCC 5. Project that have checks like that should arrive in 
2010...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107304

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

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

> I still don't fully understand the original comment from Joerg. The encoding 
> of `L'a'` cannot change at runtime; it's a literal whose encoding is decided 
> entirely at compile time. @joerg -- did you mean that Clang produces a 
> different literal encoding depending on the environment the host compiler is 
> running in?

It should, which is exactly why I consider non-trivial wchar_t literals 
fundamentally flawed as concept. AFAICT the only somewhat portable value is \0.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106577

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

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

In D106577#2899715 , @aaron.ballman 
wrote:

> In D106577#2899711 , @joerg wrote:
>
>> This patch is certainly wrong for NetBSD as the wchar_t encoding is up to 
>> the specific locale charset and *not* UCS-2 or UCS-4 for certain legacy 
>> encodings like the various shift encodings in East Asia.
>
> How does the value of a macro get impacted by a runtime locale?

NetBSD doesn't set the macro. And yes, this is one of the fundamental design 
issues of long char literals. Section 2 of the following now 20 year old Itojun 
paper goes into some of the problems with the assumption of a single universal 
character set:
https://www.usenix.org/legacy/publications/library/proceedings/usenix01/freenix01/full_papers/hagino/hagino.pdf
Even an encoding that embeds ISO 10646 fully and uses a flag bit to denote 
values (entirely valid as Unicode is restricted to 21bit) should not get this 
macro set.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106577

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

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

This patch is certainly wrong for NetBSD as the wchar_t encoding is up to the 
specific locale charset and *not* UCS-2 or UCS-4 for certain legacy encodings 
like the various shift encodings in East Asia.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106577

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

[PATCH] D104680: [clang] Eliminate relational function pointer comparisons in all C++ modes

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Is there any reason for breaking C++03 code here? I don't see the advantage to 
that at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104680

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

[PATCH] D100118: [clang] Add support for new builtin __arithmetic_fence to control floating point optimization, and new clang option fprotect-parens

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Thanks, that's better. The clang front-end option is not directly relevant for 
the semantic of the intrinsic, so it would be better to explicitly specify it 
was not being fuseable unless the operand degenerates into a single operand. 
Otherwise the specification sounds reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100118

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

[PATCH] D100118: [clang] Add support for new builtin __arithmetic_fence to control floating point optimization, and new clang option fprotect-parens

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added inline comments.



Comment at: clang/docs/UsersManual.rst:1484
+   Where unsafe floating point optimizations are enabled, this option
+   determines whether the optimizer honors parentheses when floating-point
+   expressions are evaluated.  If unsafe floating point optimizations are

aaron.ballman wrote:
> mibintc wrote:
> > aaron.ballman wrote:
> > > We may need to expound on what "honor parentheses" means. The summary for 
> > > the patch mentions `(a + b) + c`, but does this also mean `(x + y) * z`  
> > > could be interpreted as `x + (y * z)`? What about for non-arithmetic 
> > > expressions involving parentheses, like `(float1 == float2 && float1 == 
> > > float3) || blah()`?
> > Thanks for your review. Just a quick first response, What do you mean "(x + 
> > y) * z could be reinterpreted as x + (y * z)" -- not following you here? 
> "whether the optimizer honors the parentheses" reads to me like `(x + y) * z` 
> could either honor or not honor the parentheses, so that could either 
> evaluate as `(x + y) * z` or `x + (y * z)` depending on whether the parens 
> are honored or not.
I would prefer to start with a description of the intended behavior from first 
principles. The logic here should not be tied to fast-mode as it also overlaps 
a lot with the formulation of fused multiply-add expressions and certainly 
should have specified behavior on the interaction. I'm also not sure about what 
you mean with value-safe FP modes, IIRC the C/C++ standard explicitly leave it 
open in which order floating point expressions of the same operand priority are 
evaluated.

I'm also a bit puzzled why __arithmethic_fence should not be considered a 
constant expression?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100118

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

[PATCH] D103184: [AArch64] handle -Wa,-march=

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added inline comments.



Comment at: clang/lib/Driver/ToolChains/Arch/AArch64.cpp:217
+  // after -march. And while only using the the value of last -march, it
+  // includes all the options passed via -Wa,-march.
+  success = true;

This comment is confusing. `-march` is a driver option and the GCC driver will 
internally convert it into the equivalent of `-Wa,-march` before passing the 
total command line to GNU as.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103184

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

[PATCH] D101122: introduce flag -fsanitize-address-detect-stack-use-after-return-mode. No functional change.

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

There are two approaches that work for reviewing: starting from clang and going 
down or starting from compiler-rt and going up the layers. I'd prefer to do the 
latter as it means meaningful testing can be done easier. There are two natural 
steps here: clang flag+IR generation is one, llvm codegen and compiler-rt is 
the other. The clang step should include tests that ensure that proper IR is 
generated for the different flag combination (including documentation for the 
IR changes). The LLVM step should ensure that the different attributes (either 
function or module scope) are correctly lowered in the instrumentation pass and 
unit tests on the compiler-rt side to do functional testing. The unit testing 
might also be done as a third step if it goes the full way from C to binary 
code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101122

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

[PATCH] D101122: introduce flag -fsanitize-address-detect-stack-use-after-return-mode. No functional change.

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

This review doesn't make sense to me. Why add a flag that isn't affecting 
anything? Why add a flag that isn't even tested?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101122

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

[PATCH] D93031: Enable fexec-charset option

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

"Keeping the original spelling around" would assume that the input is not using 
a stateful encoding. That seems worse as assumption than giving the canonical 
output in UTF-8 and shifting the problem to the user's editor?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93031

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

[PATCH] D100054: Handle flags such as -m32 when computing the triple prefix for programs

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

This sounds wrong. If you are using 'x86_64-freebsd' as triple and -m32, it 
should still call 'x86_64-freebsd-ld', but it is the responsibility of the 
driver to ensure that also the right set of linker flags are passed as well. 
Compare `netbsd::Linker::ConstructJob` for one way to handle this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100054

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

[PATCH] D98574: [Sparc] Define the same macros for -mcpu=v9 as GCC on Linux and the BSDs

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

The NetBSD part looks ok, but also lacks proper testing. I'm not sure anyone 
but Linux cares at all otherwise as they lack 32bit SPARC support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98574

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

[PATCH] D96070: [clang] [driver] Enable static linking to libc++

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

I can't speak for other systems, but forcing libpthread to be linked is in 
general not desirable as it creates a non-trivial runtime cost, especially when 
 is not used. That's still a pretty common use case of C++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96070

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

[PATCH] D96070: [clang] [driver] Enable static linking to libc++

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

The NetBSD part is most definitely not acceptable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96070

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

[PATCH] D94355: [SimplifyCFG] Add relative switch lookup tables

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

First of all, I find this patch to be nearly impossible to read. It seems to 
mix a lot of refactoring with a functional change, making it very hard to focus 
on the core.

The main difference to the jump table logic is that the latter knows that all 
referenced addresses are within a function and therefore well contained. 
Nothing of the like seems to be found here. E.g. if this is supposed to address 
only unnamed pointers, it should be grouping them together and compute the 
offsets and then pick the optimal size. That's a transformation that can be 
beneficial for all modes for a not too large table. But it is hard to see what 
is going on here with all the seemingly unrelated changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D92307: [analyzer][StdLibraryFunctionsChecker] Fix typos in summaries of mmap and mmap64

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

off_t is s signed type. Please fix the description.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92307

___
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

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

The difference is whether we promise to be instruction precise or not. I'm not 
sure we do or want to promise that as default.


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

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

I have no problem with the change, but please adjust the description to take 
about -funwind-tables. I don't think we do async by default, at least not by 
design.


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] D88712: [CGBuiltin] Respect asm labels and redefine_extname for builtins with specialized emitting

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Disabling builtin handling when `asm` is enabled would be a major annoyance for 
NetBSD. The interaction between symbol renaming and TLI is complicated. There 
are cases where it would make perfect sense and others were it would be 
harmful. Example of the latter is the way SSP is implemented by inline 
functions that call a prototype renamed back to the original symbol name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88712

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

[PATCH] D87615: [X86] Fix stack alignment on 32-bit Solaris/x86

[Joerg Sonnenberger via Phabricator via cfe-commits]

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

I'm still curious about the source of the vptr diff, but that's a minor 
question, otherwise. LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87615

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

[PATCH] D87615: [clang][Driver] Force stack realignment on 32-bit Solaris/x86

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg requested changes to this revision.
joerg added a comment.
This revision now requires changes to proceed.

I don't think this is the right place for this at all. Look at 
`X86Subtarget::initSubtargetFeatures` please.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87615

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

[PATCH] D80300: [Driver] Add DEFAULT_DYLD_PREFIX and DEFAULT_RPATH to complement DEFAULT_SYSROOT

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

I don't agree with the justification at all, but it also seems that noone else 
cares about the build option creep here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80300



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

[PATCH] D80300: [Driver] Add DEFAULT_DYLD_PREFIX and DEFAULT_RPATH to complement DEFAULT_SYSROOT

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

I see that more as a short-coming in the existing DEFAULT_SYSROOT behavior and 
less an argument for making more cases like it. So the general idea is that for 
turnkey toolchains,
we want to allow customizing the "default" target of the toolchain to hard-code 
options like --sysroot, --target, -Wl,-rpath etc. Those are all related, so 
when using a different target, they no longer make sense. One way to deal with 
all those options in a consistent manner is hook into the logic for determining 
the current target, if none is explicitly specified on the command line or 
implicit from the executable name, then prepend some additional flags on the 
command line based on some cmake variable or so. This flags shouldn't trigger 
the unused argument warnings, so you can always pass -Wl,-rpath, --sysroot etc, 
independent of whether the compiler is in preprocessing / compile / assemble / 
link mode. That seems to be a more general and systematic approach than adding 
many additional build-time options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80300



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

[PATCH] D80300: [Driver] Add DEFAULT_DYLD_PREFIX and DEFAULT_RPATH to complement DEFAULT_SYSROOT

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

I do agree with the feature request, but I'm not sure about the implementation. 
It doesn't seem to work well with the cross-compiling support in the driver as 
clearly shown by the amount of tests that need patching. Is cross-compiling a 
concern for you at all? Otherwise I would suggest going a slightly different 
route and use default values iff no "-target" or "target-command" mechanism is 
active. Might need a way to temporarily disable unused flag warnings, but that 
way would be pretty much toolchain independent?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80300



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

[PATCH] D77697: libc++: adjust modulemap for non-modular C

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

@ldionne I was updating libc++ from d42baff45d9700a199982ba0ac04dbc6c6d911bb 
 and LLVM 
itself from 38aebe5c04ab4cb3695dc1bcc60b9a7b55215aff 
 to 
3d1424bc7a0e9a6f9887727e2bc93a10f50e1709 
 when it 
started failing. It likely has been present for a while.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77697



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

[PATCH] D77697: libc++: adjust modulemap for non-modular C

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

This fixes the module build of clang for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77697



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

[PATCH] D73245: Avoid using std::max_align_t in pre-C++11 mode

[Joerg Sonnenberger via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Revision".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG98f77828a98f: Avoid using std::max_align_t in pre-C++11 mode 
(authored by joerg).
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.

Changed prior to commit:
  https://reviews.llvm.org/D73245?vs=250937&id=254949#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73245

Files:
  libcxx/include/cstddef
  libcxx/include/stddef.h
  
libcxx/test/libcxx/experimental/memory/memory.resource.adaptor/memory.resource.adaptor.mem/db_deallocate.pass.cpp
  libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp
  libcxx/test/std/containers/sequences/array/array.data/data.pass.cpp
  libcxx/test/std/containers/sequences/array/array.data/data_const.pass.cpp
  libcxx/test/std/containers/sequences/array/size_and_alignment.pass.cpp
  libcxx/test/std/depr/depr.c.headers/stddef_h.pass.cpp
  libcxx/test/std/language.support/support.types/max_align_t.pass.cpp
  
libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp

Index: libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp
===
--- libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp
+++ libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp
@@ -17,6 +17,20 @@
 #include// for std::max_align_t
 #include "test_macros.h"
 
+// The following tests assume naturally aligned types exist
+// up to 64bit (double). For larger types, max_align_t should
+// give the correct alignment. For pre-C++11 testing, only
+// the lower bound is checked.
+
+#if TEST_STD_VER < 11
+struct natural_alignment {
+long t1;
+long long t2;
+double t3;
+long double t4;
+};
+#endif
+
 int main(int, char**)
 {
 {
@@ -250,9 +264,6 @@
 static_assert(std::alignment_of::value == 8, "");
 static_assert(sizeof(T1) == 16, "");
 }
-// Use alignof(std::max_align_t) below to find the max alignment instead of
-// hardcoding it, because it's different on different platforms.
-// (For example 8 on arm and 16 on x86.)
 {
 typedef std::aligned_storage<16>::type T1;
 #if TEST_STD_VER > 11
@@ -260,8 +271,15 @@
 #endif
 static_assert(std::is_trivial::value, "");
 static_assert(std::is_standard_layout::value, "");
-static_assert(std::alignment_of::value == TEST_ALIGNOF(std::max_align_t),
-  "");
+#if TEST_STD_VER >= 11
+const size_t alignment = TEST_ALIGNOF(std::max_align_t) > 16 ?
+16 : TEST_ALIGNOF(std::max_align_t);
+static_assert(std::alignment_of::value == alignment, "");
+#else
+static_assert(std::alignment_of::value >=
+  TEST_ALIGNOF(natural_alignment), "");
+static_assert(std::alignment_of::value <= 16, "");
+#endif
 static_assert(sizeof(T1) == 16, "");
 }
 {
@@ -271,9 +289,17 @@
 #endif
 static_assert(std::is_trivial::value, "");
 static_assert(std::is_standard_layout::value, "");
-static_assert(std::alignment_of::value == TEST_ALIGNOF(std::max_align_t),
-  "");
-static_assert(sizeof(T1) == 16 + TEST_ALIGNOF(std::max_align_t), "");
+#if TEST_STD_VER >= 11
+const size_t alignment = TEST_ALIGNOF(std::max_align_t) > 16 ?
+16 : TEST_ALIGNOF(std::max_align_t);
+static_assert(std::alignment_of::value == alignment, "");
+static_assert(sizeof(T1) == 16 + alignment, "");
+#else
+static_assert(std::alignment_of::value >=
+  TEST_ALIGNOF(natural_alignment), "");
+static_assert(std::alignment_of::value <= 16);
+static_assert(sizeof(T1) % TEST_ALIGNOF(natural_alignment) == 0, "");
+#endif
 }
 {
 typedef std::aligned_storage<10>::type T1;
Index: libcxx/test/std/language.support/support.types/max_align_t.pass.cpp
===
--- libcxx/test/std/language.support/support.types/max_align_t.pass.cpp
+++ libcxx/test/std/language.support/support.types/max_align_t.pass.cpp
@@ -6,6 +6,8 @@
 //
 //===--===//
 
+// UNSUPPORTED: c++98, c++03
+
 #include 
 #include 
 
@@ -41,5 +43,11 @@
   "std::alignment_of::value >= "
   "std::alignment_of::value");
 
+#ifdef __STDCPP_DEFAULT_NEW_ALIGNMENT__
+static_assert(std::alignment_of::value <=
+  __STDCPP_DEFAULT_NEW_ALIGNMENT__,
+  "max_align_t alignment is no larger than new alignment");
+#endif
+
   return 0;
 }
Index: libcxx/test/std/depr/depr.c.headers/stddef_h.pass.cpp
===
--- libcxx/test/std/depr/depr.c

[PATCH] D73245: Avoid using std::max_align_t in pre-C++11 mode

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

ping2

Louis, did I answer your questions?


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

https://reviews.llvm.org/D73245



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

[PATCH] D73245: Depend stddef.h to provide max_align_t for C++11 and provide better fallback in

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.
Herald added a subscriber: broadwaylamb.

Ping?


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

https://reviews.llvm.org/D73245



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

[PATCH] D73245: Depend stddef.h to provide max_align_t for C++11 and provide better fallback in

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg marked 2 inline comments as done.
joerg added inline comments.



Comment at: 
libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp:275
+#if TEST_STD_VER >= 11
+const size_t alignment = TEST_ALIGNOF(std::max_align_t) > 16 ?
+16 : TEST_ALIGNOF(std::max_align_t);

ldionne wrote:
> joerg wrote:
> > ldionne wrote:
> > > Can you walk me through why the check for `> 16` is required?
> > If max_align_t is 256bit, we still only expect 128bit alignment (long 
> > double). This test is still checking more than it should, e.g. in principle 
> > a target could legitimately have no natural type larger than 64bit, but 
> > support 256bit vector types and set max_align_t accordingly. The condition 
> > is here because std::min in C++11 is not constexpr.
> Naively, I would have expected the test to be:
> 
> ```
> #if TEST_STD_VER >= 11 // we know max_align_t is provided
>   static_assert(std::alignment_of::value == 
> TEST_ALIGNOF(std::max_align_t), "");
> #else
>   static_assert(std::alignment_of::value >= 
> TEST_ALIGNOF(natural_alignment), "");
> #endif
> ```
> 
> To make sure I understand, you're saying we can't do that because the 
> `std::alignment_of::value == TEST_ALIGNOF(std::max_align_t)` test would 
> sometimes fail if `alignof(std::max_align_t)` is larger than the natural 
> alignment used by `std::aligned_storage`. This also highlights that there's a 
> change in the value of `alignof(std::max_align_t)` with this change. Is that 
> correct?
Yes, the test would have failed before when `max_align_t > 16`. There is no 
change of value in `alignof(max_align_t)`, just seen by review.


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

https://reviews.llvm.org/D73245



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

[PATCH] D73245: Depend stddef.h to provide max_align_t for C++11 and provide better fallback in

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg marked 5 inline comments as done.
joerg added inline comments.



Comment at: libcxx/include/stddef.h:58
 !defined(__DEFINED_max_align_t) && !defined(__NetBSD__)
 typedef long double max_align_t;
 #endif

ldionne wrote:
> Why do we need this at all anymore? In C++11, can't we rely on the C Standard 
> Library providing it `::max_align_t`?
Sorry, left-over. Will be killed.



Comment at: 
libcxx/test/libcxx/experimental/memory/memory.resource.adaptor/memory.resource.adaptor.mem/db_deallocate.pass.cpp:40
+#ifdef __STDCPP_DEFAULT_NEW_ALIGNMENT__
+std::size_t maxSize = std::numeric_limits::max()
+- __STDCPP_DEFAULT_NEW_ALIGNMENT__;

ldionne wrote:
> This test is only enabled in >= C++11, so I think `std::max_align_t` should 
> always be provided. So why do we want that `#if`?
We know that the new alignment can be stricter than max_align_t and we want to 
test the overflow case here. So going for the strictest alignment is more 
sensible.



Comment at: 
libcxx/test/std/language.support/support.types/max_align_t.pass.cpp:47
+#ifdef __STDCPP_DEFAULT_NEW_ALIGNMENT__
+static_assert(std::alignment_of::value <=
+  __STDCPP_DEFAULT_NEW_ALIGNMENT__,

ldionne wrote:
> Since we now assume that `__STDCPP_DEFAULT_NEW_ALIGNMENT__` is defined, can 
> we remove that `#if` and keep the assertion?
It's only required in C++03 mode. We still support C++11 and higher without it, 
e.g. for gcc.



Comment at: 
libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp:275
+#if TEST_STD_VER >= 11
+const size_t alignment = TEST_ALIGNOF(std::max_align_t) > 16 ?
+16 : TEST_ALIGNOF(std::max_align_t);

ldionne wrote:
> Can you walk me through why the check for `> 16` is required?
If max_align_t is 256bit, we still only expect 128bit alignment (long double). 
This test is still checking more than it should, e.g. in principle a target 
could legitimately have no natural type larger than 64bit, but support 256bit 
vector types and set max_align_t accordingly. The condition is here because 
std::min in C++11 is not constexpr.


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

https://reviews.llvm.org/D73245



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

[PATCH] D76186: Fix compatibility for __builtin_stdarg_start

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg closed this revision.
joerg added a comment.

Committed as 0b999f76575f0196d3cd01c0781b2513b0a1af15 without link.


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

https://reviews.llvm.org/D76186



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

[PATCH] D73245: Depend stddef.h to provide max_align_t for C++11 and provide better fallback in

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg updated this revision to Diff 250937.
joerg edited the summary of this revision.
joerg added a comment.

Require `__STDCPP_NEW_ALIGNMENT__` in C++03 mode. Prefer it over `max_align_t` 
in a number of tests when allocation alignment is desired. Adjust some tests to 
do minimal sanity checking of the alignment for C++03 only. Add an explicit 
check that `__STDCPP_NEW_ALIGNMENT__`  is more general than `max_align_t` if 
both are present.


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

https://reviews.llvm.org/D73245

Files:
  libcxx/include/cstddef
  libcxx/include/stddef.h
  
libcxx/test/libcxx/experimental/memory/memory.resource.adaptor/memory.resource.adaptor.mem/db_deallocate.pass.cpp
  libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp
  libcxx/test/std/containers/sequences/array/array.data/data.pass.cpp
  libcxx/test/std/containers/sequences/array/array.data/data_const.pass.cpp
  libcxx/test/std/containers/sequences/array/size_and_alignment.pass.cpp
  libcxx/test/std/depr/depr.c.headers/stddef_h.pass.cpp
  libcxx/test/std/language.support/support.types/max_align_t.pass.cpp
  
libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp

Index: libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp
===
--- libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp
+++ libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp
@@ -17,6 +17,20 @@
 #include// for std::max_align_t
 #include "test_macros.h"
 
+// The following tests assume naturally aligned types exist
+// up to 64bit (double). For larger types, max_align_t should
+// give the correct alignment. For pre-C++11 testing, only
+// the lower bound is checked.
+
+#if TEST_STD_VER < 11
+struct natural_alignment {
+long t1;
+long long t2;
+double t3;
+long double t4;
+};
+#endif
+
 int main(int, char**)
 {
 {
@@ -250,9 +264,6 @@
 static_assert(std::alignment_of::value == 8, "");
 static_assert(sizeof(T1) == 16, "");
 }
-// Use alignof(std::max_align_t) below to find the max alignment instead of
-// hardcoding it, because it's different on different platforms.
-// (For example 8 on arm and 16 on x86.)
 {
 typedef std::aligned_storage<16>::type T1;
 #if TEST_STD_VER > 11
@@ -260,8 +271,15 @@
 #endif
 static_assert(std::is_trivial::value, "");
 static_assert(std::is_standard_layout::value, "");
-static_assert(std::alignment_of::value == TEST_ALIGNOF(std::max_align_t),
-  "");
+#if TEST_STD_VER >= 11
+const size_t alignment = TEST_ALIGNOF(std::max_align_t) > 16 ?
+16 : TEST_ALIGNOF(std::max_align_t);
+static_assert(std::alignment_of::value == alignment, "");
+#else
+static_assert(std::alignment_of::value >=
+  TEST_ALIGNOF(natural_alignment), "");
+static_assert(std::alignment_of::value <= 16, "");
+#endif
 static_assert(sizeof(T1) == 16, "");
 }
 {
@@ -271,9 +289,17 @@
 #endif
 static_assert(std::is_trivial::value, "");
 static_assert(std::is_standard_layout::value, "");
-static_assert(std::alignment_of::value == TEST_ALIGNOF(std::max_align_t),
-  "");
-static_assert(sizeof(T1) == 16 + TEST_ALIGNOF(std::max_align_t), "");
+#if TEST_STD_VER >= 11
+const size_t alignment = TEST_ALIGNOF(std::max_align_t) > 16 ?
+16 : TEST_ALIGNOF(std::max_align_t);
+static_assert(std::alignment_of::value == alignment, "");
+static_assert(sizeof(T1) == 16 + alignment, "");
+#else
+static_assert(std::alignment_of::value >=
+  TEST_ALIGNOF(natural_alignment), "");
+static_assert(std::alignment_of::value <= 16);
+static_assert(sizeof(T1) % TEST_ALIGNOF(natural_alignment) == 0, "");
+#endif
 }
 {
 typedef std::aligned_storage<10>::type T1;
Index: libcxx/test/std/language.support/support.types/max_align_t.pass.cpp
===
--- libcxx/test/std/language.support/support.types/max_align_t.pass.cpp
+++ libcxx/test/std/language.support/support.types/max_align_t.pass.cpp
@@ -6,6 +6,8 @@
 //
 //===--===//
 
+// UNSUPPORTED: c++98, c++03
+
 #include 
 #include 
 
@@ -41,5 +43,11 @@
   "std::alignment_of::value >= "
   "std::alignment_of::value");
 
+#ifdef __STDCPP_DEFAULT_NEW_ALIGNMENT__
+static_assert(std::alignment_of::value <=
+  __STDCPP_DEFAULT_NEW_ALIGNMENT__,
+  "max_align_t alignment is no larger than new alignment");
+#endif
+
   return 0;
 }
Index: libcxx/test/std/depr/depr.c.headers/stddef_h.pass.cpp
===
--- libcxx/test/std/depr/depr.c.headers/stddef_h.pass.cpp
+++ libcxx/test/std

[PATCH] D76186: Fix compatibility for __builtin_stdarg_start

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg created this revision.

The `__builtin_stdarg_start` is the legacy spelling of `__builtin_va_start`. It 
should behave exactly the same, but for the last 9 years it would behave subtly 
different for diagnostics. Follow the change from 29ad95b23217 to require 
custom type checking.


https://reviews.llvm.org/D76186

Files:
  clang/include/clang/Basic/Builtins.def
  clang/test/SemaCXX/vararg-non-pod.cpp


Index: clang/test/SemaCXX/vararg-non-pod.cpp
===
--- clang/test/SemaCXX/vararg-non-pod.cpp
+++ clang/test/SemaCXX/vararg-non-pod.cpp
@@ -164,6 +164,13 @@
   __builtin_va_start(list, somearg);
 }
 
+// __builtin_stdarg_start is a compatibility alias for __builtin_va_start,
+// it should behave the same
+void t6b(Foo somearg, ... ) {
+  __builtin_va_list list;
+  __builtin_stdarg_start(list, somearg); // second argument to 'va_start' is 
not the last named parameter [-Wvarargs]
+}
+
 void t7(int n, ...) {
   __builtin_va_list list;
   __builtin_va_start(list, n);
Index: clang/include/clang/Basic/Builtins.def
===
--- clang/include/clang/Basic/Builtins.def
+++ clang/include/clang/Basic/Builtins.def
@@ -471,7 +471,7 @@
 BUILTIN(__builtin_va_start, "vA.", "nt")
 BUILTIN(__builtin_va_end, "vA", "n")
 BUILTIN(__builtin_va_copy, "vAA", "n")
-BUILTIN(__builtin_stdarg_start, "vA.", "n")
+BUILTIN(__builtin_stdarg_start, "vA.", "nt")
 BUILTIN(__builtin_assume_aligned, "v*vC*z.", "nc")
 BUILTIN(__builtin_bcmp, "ivC*vC*z", "Fn")
 BUILTIN(__builtin_bcopy, "vv*v*z", "n")


Index: clang/test/SemaCXX/vararg-non-pod.cpp
===
--- clang/test/SemaCXX/vararg-non-pod.cpp
+++ clang/test/SemaCXX/vararg-non-pod.cpp
@@ -164,6 +164,13 @@
   __builtin_va_start(list, somearg);
 }
 
+// __builtin_stdarg_start is a compatibility alias for __builtin_va_start,
+// it should behave the same
+void t6b(Foo somearg, ... ) {
+  __builtin_va_list list;
+  __builtin_stdarg_start(list, somearg); // second argument to 'va_start' is not the last named parameter [-Wvarargs]
+}
+
 void t7(int n, ...) {
   __builtin_va_list list;
   __builtin_va_start(list, n);
Index: clang/include/clang/Basic/Builtins.def
===
--- clang/include/clang/Basic/Builtins.def
+++ clang/include/clang/Basic/Builtins.def
@@ -471,7 +471,7 @@
 BUILTIN(__builtin_va_start, "vA.", "nt")
 BUILTIN(__builtin_va_end, "vA", "n")
 BUILTIN(__builtin_va_copy, "vAA", "n")
-BUILTIN(__builtin_stdarg_start, "vA.", "n")
+BUILTIN(__builtin_stdarg_start, "vA.", "nt")
 BUILTIN(__builtin_assume_aligned, "v*vC*z.", "nc")
 BUILTIN(__builtin_bcmp, "ivC*vC*z", "Fn")
 BUILTIN(__builtin_bcopy, "vv*v*z", "n")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73245: Depend stddef.h to provide max_align_t for C++11 and provide better fallback in

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added inline comments.



Comment at: libcxx/include/new:240
   return __align > __STDCPP_DEFAULT_NEW_ALIGNMENT__;
+#elif defined(_LIBCPP_CXX03_LANG)
+  return __align > alignment_of<__libcpp_max_align_t>::value;

ldionne wrote:
> So, IIUC what you're saying, `__STDCPP_DEFAULT_NEW_ALIGNMENT__` is provided 
> by recent Clangs in C++03 mode? I tested it and it does seem to be correct. 
> (side note: I tend to think we should be more aggressive to remove old 
> compiler support, since most people don't upgrade their stdlib without 
> upgrading their compiler anyway).
> 
> So if we don't care about supporting old compilers that don't provide that 
> macro, we could just get rid of this `#elif`, and such compilers would error 
> out when trying to use `max_align_t` in the `#else` below. That appears 
> reasonable to me.
> 
> We'd still leave the `#if TEST_STD_VER >= 11` in the tests below, since in 
> C++03 we wouldn't provide `std::max_align_t`, however testing that we use 
> overaligned new in the same conditions in C++03 and C++11 becomes trivial, 
> because it's the same code path.
> 
> Did I get what you meant correctly? If so, that sounds like a viable path 
> forward to me, since we're simplifying the code. We're also improving on our 
> C++03 conformance, which isn't considered good but is certainly not 
> considered bad either.
Correct, it has been provided since clang 4.0 at least it seems. For testing, 
we have two cases, some that specifically check properties of max_align_t and 
those should be restricted to C++11 and newer. I think we should grow a new 
check that max_align_t <= __STDCPP_DEFAULT_NEW_ALIGNMENT__ as sanity check, but 
that's slightly OT. Most of the existing cases to check for overalignment 
already use __STDCPP_DEFAULT_NEW_ALIGNMENT__ anyway, so it would be a 
case-by-case check for those.


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

https://reviews.llvm.org/D73245



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

[PATCH] D73245: Depend stddef.h to provide max_align_t for C++11 and provide better fallback in

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

In D73245#1918287 , @ldionne wrote:

> In D73245#1918148 , @EricWF wrote:
>
> > I've already stated my disapproval of this patch. Libc++ has never and will 
> > never provide nor value C++03 conformance.
> >  Moving backwards to C++03 is inconsistent with the libraries general 
> > direction.
>
>
> @EricWF makes a point here, we want to move away from C++03.


Removing or breaking C++03 compatibility is actively harmful for that. Speaking 
as a system integrator, a STL implementation that can't deal in a reasonable 
way with the existing C++03 code base is useless. It's a nice ivory tower 
assumption that old code will just disappear if you wish for it enough, but it 
doesn't reflect well on the reality. Especially given the various ways can and 
does fail when compiled for C++11 or later. Having to switch between different 
STL implementations doesn't help anyone in the process of modernizing a code 
base, especially when interacting with code that needs large semi-intrusive 
changes to work in C++11 mode. As a most basic issue, it makes it much harder 
to separate issues with the STL implementation (i.e. depending on 
implementation-defined behavior) from semantic changes in the core language 
(i.e. noexcept dtors, auto) and mechanical changes (e.g. UDL)

>> This patch disables tests, which could hide bugs, including serious ABI 
>> differences between dialects.
>> 
>> I would like to unbreak compilation on NetBSD. But all that's needed there 
>> is to provide our own correct declaration of max_align_t.
>>  I don't see why C++03 conformance is a necessary condition.
> 
> Is there anything that can be done on the NetBSD side to solve this? 
> Basically, just imagine that libc++ doesn't provide a C++03 mode at all -- 
> what would you do then? I think that's the right mindset to solve this 
> specific problem.

Can you both please go back to look at the core of the issue? The current 
behavior of libc++ is at best an ODR violation on ALL platforms. This is not a 
problem specific to NetBSD at all, it has just been exposed as a different 
error on NetBSD. The documented behavior of libc++ is to provide C++03 
compatibility as much as possible when it doesn't conflict with C++11 and the 
compiler (clang) supports the necessary language feature. max_align_t is not 
provided by clang in C++03 nor is it provided by GCC.  Now there are different 
options for fixes, but it is very frustrating to me that the primary feedback 
so far is "Don't use C++03" and therefore not helpful at all.  As I said 
before, libc++ cares about max_align_t exactly for one purpose: whether 
overaligned types need special allocators. Now C++03 normally can't depend on 
the implementation to do something useful in this case, simply because the 
environment doesn't have the necessary means. So it all boils down to what is 
the least intrusive and potentially most well testable mechanism. The core of 
the patch provides one answer for that. If the compiler supports telling us the 
default alignment for new, use it just like we would in C++11 mode. A 
constructive feedback would now be an answer to the question of whether we care 
about libc++ use with compilers that do not provide that macro for C++03 mode. 
I don't know how useful libc++ in C++03 mode is for any compiler except clang 
and as I wrote, we document the primary target being clang. As such the primary 
question is whether we care about compatibility with older versions of clang 
and modern libc++.  If we don't, the change to `` becomes much simpler as 
it can just bail out and the corresponding testing becomes easier as well, 
since it would only have to assert that max_align_t is aligned less strictly 
than the platform allocator. It would solve the core issue that we provide a 
max_align_t when we don't know what it should be and actively disagree with the 
max_align_t we provide for the other dialects.


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

https://reviews.llvm.org/D73245



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

[PATCH] D73245: Depend stddef.h to provide max_align_t for C++11 and provide better fallback in

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Ping


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

https://reviews.llvm.org/D73245



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

[PATCH] D73245: Depend stddef.h to provide max_align_t for C++11 and provide better fallback in

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

libc++ has no idea what a correct max_align_t is. The internal definition works 
due to historic requirements that all three fundamental types are supported for 
new/delete, but we don't have any such guarantees for every other context. A 
correctly implemented stddef.h does not provide max_align_t in C++03 mode, 
since that would pollute the global namespace. This means that libc++ currently 
has two failure modes: on NetBSD, it outright tries to use a non-existing 
symbol. On other platforms it silently defines max_align_t in a way that can be 
subtle wrong.

I should add that e.g. libstdc++ doesn't provide it either, so at least 
somewhat portable C++03 code can not depend on the presence anyway.


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

https://reviews.llvm.org/D73245



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

[PATCH] D73245: Depend stddef.h to provide max_align_t for C++11 and provide better fallback in

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg updated this revision to Diff 246524.
joerg added a comment.

Do not depend on max_align_t in C++03 mode in the test cases.


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

https://reviews.llvm.org/D73245

Files:
  libcxx/include/cstddef
  libcxx/include/new
  libcxx/include/stddef.h
  libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp
  libcxx/test/std/containers/sequences/array/array.data/data.pass.cpp
  libcxx/test/std/containers/sequences/array/array.data/data_const.pass.cpp
  libcxx/test/std/containers/sequences/array/size_and_alignment.pass.cpp
  libcxx/test/std/depr/depr.c.headers/stddef_h.pass.cpp
  libcxx/test/std/language.support/support.types/max_align_t.pass.cpp
  
libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp

Index: libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp
===
--- libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp
+++ libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp
@@ -260,9 +260,11 @@
 #endif
 static_assert(std::is_trivial::value, "");
 static_assert(std::is_standard_layout::value, "");
+#if TEST_STD_VER >= 11
 static_assert(std::alignment_of::value == TEST_ALIGNOF(std::max_align_t),
   "");
 static_assert(sizeof(T1) == 16, "");
+#endif
 }
 {
 typedef std::aligned_storage<17>::type T1;
@@ -271,9 +273,11 @@
 #endif
 static_assert(std::is_trivial::value, "");
 static_assert(std::is_standard_layout::value, "");
+#if TEST_STD_VER >= 11
 static_assert(std::alignment_of::value == TEST_ALIGNOF(std::max_align_t),
   "");
 static_assert(sizeof(T1) == 16 + TEST_ALIGNOF(std::max_align_t), "");
+#endif
 }
 {
 typedef std::aligned_storage<10>::type T1;
Index: libcxx/test/std/language.support/support.types/max_align_t.pass.cpp
===
--- libcxx/test/std/language.support/support.types/max_align_t.pass.cpp
+++ libcxx/test/std/language.support/support.types/max_align_t.pass.cpp
@@ -18,6 +18,7 @@
 int main(int, char**)
 {
 
+#if TEST_STD_VER >= 11
 #if TEST_STD_VER > 17
 //  P0767
 static_assert(std::is_trivial::value,
@@ -40,6 +41,7 @@
   std::alignment_of::value,
   "std::alignment_of::value >= "
   "std::alignment_of::value");
+#endif
 
   return 0;
 }
Index: libcxx/test/std/depr/depr.c.headers/stddef_h.pass.cpp
===
--- libcxx/test/std/depr/depr.c.headers/stddef_h.pass.cpp
+++ libcxx/test/std/depr/depr.c.headers/stddef_h.pass.cpp
@@ -49,10 +49,11 @@
   "std::is_trivial::value");
 static_assert(std::is_standard_layout::value,
   "std::is_standard_layout::value");
-#else
+#elif TEST_STD_VER >= 11
 static_assert(std::is_pod::value,
   "std::is_pod::value");
 #endif
+#if TEST_STD_VER >= 11
 static_assert((std::alignment_of::value >=
   std::alignment_of::value),
   "std::alignment_of::value >= "
@@ -65,6 +66,7 @@
   std::alignment_of::value,
   "std::alignment_of::value >= "
   "std::alignment_of::value");
+#endif
 
   return 0;
 }
Index: libcxx/test/std/containers/sequences/array/size_and_alignment.pass.cpp
===
--- libcxx/test/std/containers/sequences/array/size_and_alignment.pass.cpp
+++ libcxx/test/std/containers/sequences/array/size_and_alignment.pass.cpp
@@ -49,6 +49,7 @@
   test();
 }
 
+#if TEST_STD_VER >= 11
 struct TEST_ALIGNAS(TEST_ALIGNOF(std::max_align_t) * 2) TestType1 {
 
 };
@@ -56,6 +57,7 @@
 struct TEST_ALIGNAS(TEST_ALIGNOF(std::max_align_t) * 2) TestType2 {
   char data[1000];
 };
+#endif
 
 //static_assert(sizeof(void*) == 4, "");
 
@@ -64,9 +66,11 @@
   test_type();
   test_type();
   test_type();
+#if TEST_STD_VER >= 11
   test_type();
   test_type();
   test_type();
+#endif
 
   return 0;
 }
Index: libcxx/test/std/containers/sequences/array/array.data/data_const.pass.cpp
===
--- libcxx/test/std/containers/sequences/array/array.data/data_const.pass.cpp
+++ libcxx/test/std/containers/sequences/array/array.data/data_const.pass.cpp
@@ -49,6 +49,7 @@
   const T* p = c.data();
   LIBCPP_ASSERT(p != nullptr);
 }
+#if TEST_STD_VER >= 11
 {
   typedef std::max_align_t T;
   typedef std::array C;
@@ -58,6 +59,7 @@
   std::uintptr_t pint = reinterpret_cast(p);
   assert(pint % TEST_ALIGNOF(std::max_align_t) == 0);
 }
+#endif
 #if TEST_STD_VER > 14
 {
 typedef std::array C;
Index: libcxx/test/std/containers/sequences/array/array.data/data.pass.cpp
===

[PATCH] D73245: Depend stddef.h to provide max_align_t for C++11 and provide better fallback in

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg updated this revision to Diff 245853.

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

https://reviews.llvm.org/D73245

Files:
  libcxx/include/cstddef
  libcxx/include/new
  libcxx/include/stddef.h


Index: libcxx/include/stddef.h
===
--- libcxx/include/stddef.h
+++ libcxx/include/stddef.h
@@ -51,12 +51,6 @@
 using std::nullptr_t;
 }
 
-// Re-use the compiler's  max_align_t where possible.
-#if !defined(__CLANG_MAX_ALIGN_T_DEFINED) && !defined(_GCC_MAX_ALIGN_T) && \
-!defined(__DEFINED_max_align_t) && !defined(__NetBSD__)
-typedef long double max_align_t;
-#endif
-
 #endif
 
 #endif  // _LIBCPP_STDDEF_H
Index: libcxx/include/new
===
--- libcxx/include/new
+++ libcxx/include/new
@@ -226,9 +226,19 @@
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
+#if defined(_LIBCPP_CXX03_LANG)
+union __libcpp_max_align_t {
+  void * __f1;
+  long long int __f2;
+  long double __f3;
+};
+#endif
+
 _LIBCPP_CONSTEXPR inline _LIBCPP_INLINE_VISIBILITY bool 
__is_overaligned_for_new(size_t __align) _NOEXCEPT {
 #ifdef __STDCPP_DEFAULT_NEW_ALIGNMENT__
   return __align > __STDCPP_DEFAULT_NEW_ALIGNMENT__;
+#elif defined(_LIBCPP_CXX03_LANG)
+  return __align > alignment_of<__libcpp_max_align_t>::value;
 #else
   return __align > alignment_of::value;
 #endif
Index: libcxx/include/cstddef
===
--- libcxx/include/cstddef
+++ libcxx/include/cstddef
@@ -25,7 +25,7 @@
 
 ptrdiff_t
 size_t
-max_align_t
+max_align_t // C++11
 nullptr_t
 byte // C++17
 
@@ -49,12 +49,8 @@
 using ::ptrdiff_t;
 using ::size_t;
 
-#if defined(__CLANG_MAX_ALIGN_T_DEFINED) || defined(_GCC_MAX_ALIGN_T) || \
-defined(__DEFINED_max_align_t) || defined(__NetBSD__)
-// Re-use the compiler's  max_align_t where possible.
+#if !defined(_LIBCPP_CXX03_LANG)
 using ::max_align_t;
-#else
-typedef long double max_align_t;
 #endif
 
 template  struct __libcpp_is_integral { enum { 
value = 0 }; };


Index: libcxx/include/stddef.h
===
--- libcxx/include/stddef.h
+++ libcxx/include/stddef.h
@@ -51,12 +51,6 @@
 using std::nullptr_t;
 }
 
-// Re-use the compiler's  max_align_t where possible.
-#if !defined(__CLANG_MAX_ALIGN_T_DEFINED) && !defined(_GCC_MAX_ALIGN_T) && \
-!defined(__DEFINED_max_align_t) && !defined(__NetBSD__)
-typedef long double max_align_t;
-#endif
-
 #endif
 
 #endif  // _LIBCPP_STDDEF_H
Index: libcxx/include/new
===
--- libcxx/include/new
+++ libcxx/include/new
@@ -226,9 +226,19 @@
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
+#if defined(_LIBCPP_CXX03_LANG)
+union __libcpp_max_align_t {
+  void * __f1;
+  long long int __f2;
+  long double __f3;
+};
+#endif
+
 _LIBCPP_CONSTEXPR inline _LIBCPP_INLINE_VISIBILITY bool __is_overaligned_for_new(size_t __align) _NOEXCEPT {
 #ifdef __STDCPP_DEFAULT_NEW_ALIGNMENT__
   return __align > __STDCPP_DEFAULT_NEW_ALIGNMENT__;
+#elif defined(_LIBCPP_CXX03_LANG)
+  return __align > alignment_of<__libcpp_max_align_t>::value;
 #else
   return __align > alignment_of::value;
 #endif
Index: libcxx/include/cstddef
===
--- libcxx/include/cstddef
+++ libcxx/include/cstddef
@@ -25,7 +25,7 @@
 
 ptrdiff_t
 size_t
-max_align_t
+max_align_t // C++11
 nullptr_t
 byte // C++17
 
@@ -49,12 +49,8 @@
 using ::ptrdiff_t;
 using ::size_t;
 
-#if defined(__CLANG_MAX_ALIGN_T_DEFINED) || defined(_GCC_MAX_ALIGN_T) || \
-defined(__DEFINED_max_align_t) || defined(__NetBSD__)
-// Re-use the compiler's  max_align_t where possible.
+#if !defined(_LIBCPP_CXX03_LANG)
 using ::max_align_t;
-#else
-typedef long double max_align_t;
 #endif
 
 template  struct __libcpp_is_integral { enum { value = 0 }; };
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73245: Depend stddef.h to provide max_align_t for C++11 and provide better fallback in

[Joerg Sonnenberger via Phabricator via cfe-commits]

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



Comment at: libcxx/include/new:229-237
+#if !defined(_LIBCPP_CXX03_LANG)
+using __libcpp_max_align_t = max_align_t;
+#else
+union __libcpp_max_align_t {
+  void * __f1;
+  long long int __f2;
+  long double __f3;

EricWF wrote:
> rsmith wrote:
> > Is there any ODR risk from this, or similar? Does libc++ support building 
> > in mixed C++98 / C++11 mode? If different TUs disagree on this alignment, 
> > we can end up allocating with the aligned allocator and deallocating with 
> > the unaligned allocator, which is not guaranteed to work.
> > 
> > We could always use the union approach if we don't know the default new 
> > alignment. But from the code below it looks like we might only ever use 
> > this if we have aligned allocation support, in which case we can just 
> > assume that the default new alignment is defined. So perhaps we can just 
> > hide the entire definition of `__is_overaligned_for_new` behind a `#ifdef 
> > __STDCPP_DEFAULT_NEW_ALIGNMENT__` and never even consider `max_align_t`?
> `max_align_t` should be ABI stable. I would rather we copy/paste the clang 
> definition into the libc++ sources so we can use it when it's not provided by 
> the compiler.
> 
> Though this raises another can of worms because GCC and Clang don't agree on 
> a size for `max_align_t`.
max_align_t doesn't exist in C++03 mode, the included version is the nearest 
thing possible in portable C++. A visible difference happens if:
(1) The user requires C++03
(2) The code contains non-standard types with either explicit alignment or 
larger implicit alignment than the base types.
(3) __STDCPP_DEFAULT_NEW_ALIGNMENT__ or alignof(max_align_t) is larger than the 
alignment of the union.
In that case, behavior changes as to which allocator/deallocator is used. If 
the explicit aligned version is not compatible and life-time crosses into c++11 
mode, it could be a problem. But at that point I think we did all we could 
possible do to provide compatibility and the code is too far in 
implementation-defined land already.


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

https://reviews.llvm.org/D73245



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

[PATCH] D73245: Depend stddef.h to provide max_align_t for C++11 and provide better fallback in

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

It is used both in `` and `` and the use in the latter is 
currently unconditional AFAICT. I don't have a problem splitting the 
conditional to avoid the typedef. That would address the ODR concern?


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

https://reviews.llvm.org/D73245



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

[PATCH] D73245: Depend stddef.h to provide max_align_t for C++11 and provide better fallback in

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Ping2?


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

https://reviews.llvm.org/D73245



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

[PATCH] D73245: Depend stddef.h to provide max_align_t for C++11 and provide better fallback in

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Ping?


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

https://reviews.llvm.org/D73245



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

[PATCH] D73245: Depend stddef.h to provide max_align_t for C++11 and provide better fallback in

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg updated this revision to Diff 242317.
joerg retitled this revision from "Don't define std::max_align_t if not used in 
C++03 mode" to "Depend stddef.h to provide max_align_t for C++11 and provide 
better fallback in ".
joerg edited the summary of this revision.
Herald added a subscriber: krytarowski.

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

https://reviews.llvm.org/D73245

Files:
  libcxx/include/cstddef
  libcxx/include/new
  libcxx/include/stddef.h


Index: libcxx/include/stddef.h
===
--- libcxx/include/stddef.h
+++ libcxx/include/stddef.h
@@ -51,12 +51,6 @@
 using std::nullptr_t;
 }
 
-// Re-use the compiler's  max_align_t where possible.
-#if !defined(__CLANG_MAX_ALIGN_T_DEFINED) && !defined(_GCC_MAX_ALIGN_T) && \
-!defined(__DEFINED_max_align_t) && !defined(__NetBSD__)
-typedef long double max_align_t;
-#endif
-
 #endif
 
 #endif  // _LIBCPP_STDDEF_H
Index: libcxx/include/new
===
--- libcxx/include/new
+++ libcxx/include/new
@@ -226,11 +226,21 @@
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
+#if !defined(_LIBCPP_CXX03_LANG)
+using __libcpp_max_align_t = max_align_t;
+#else
+union __libcpp_max_align_t {
+  void * __f1;
+  long long int __f2;
+  long double __f3;
+};
+#endif
+
 _LIBCPP_CONSTEXPR inline _LIBCPP_INLINE_VISIBILITY bool 
__is_overaligned_for_new(size_t __align) _NOEXCEPT {
 #ifdef __STDCPP_DEFAULT_NEW_ALIGNMENT__
   return __align > __STDCPP_DEFAULT_NEW_ALIGNMENT__;
 #else
-  return __align > alignment_of::value;
+  return __align > alignment_of<__libcpp_max_align_t>::value;
 #endif
 }
 
Index: libcxx/include/cstddef
===
--- libcxx/include/cstddef
+++ libcxx/include/cstddef
@@ -25,7 +25,7 @@
 
 ptrdiff_t
 size_t
-max_align_t
+max_align_t // C++11
 nullptr_t
 byte // C++17
 
@@ -49,12 +49,8 @@
 using ::ptrdiff_t;
 using ::size_t;
 
-#if defined(__CLANG_MAX_ALIGN_T_DEFINED) || defined(_GCC_MAX_ALIGN_T) || \
-defined(__DEFINED_max_align_t) || defined(__NetBSD__)
-// Re-use the compiler's  max_align_t where possible.
+#if !defined(_LIBCPP_CXX03_LANG)
 using ::max_align_t;
-#else
-typedef long double max_align_t;
 #endif
 
 _LIBCPP_END_NAMESPACE_STD


Index: libcxx/include/stddef.h
===
--- libcxx/include/stddef.h
+++ libcxx/include/stddef.h
@@ -51,12 +51,6 @@
 using std::nullptr_t;
 }
 
-// Re-use the compiler's  max_align_t where possible.
-#if !defined(__CLANG_MAX_ALIGN_T_DEFINED) && !defined(_GCC_MAX_ALIGN_T) && \
-!defined(__DEFINED_max_align_t) && !defined(__NetBSD__)
-typedef long double max_align_t;
-#endif
-
 #endif
 
 #endif  // _LIBCPP_STDDEF_H
Index: libcxx/include/new
===
--- libcxx/include/new
+++ libcxx/include/new
@@ -226,11 +226,21 @@
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
+#if !defined(_LIBCPP_CXX03_LANG)
+using __libcpp_max_align_t = max_align_t;
+#else
+union __libcpp_max_align_t {
+  void * __f1;
+  long long int __f2;
+  long double __f3;
+};
+#endif
+
 _LIBCPP_CONSTEXPR inline _LIBCPP_INLINE_VISIBILITY bool __is_overaligned_for_new(size_t __align) _NOEXCEPT {
 #ifdef __STDCPP_DEFAULT_NEW_ALIGNMENT__
   return __align > __STDCPP_DEFAULT_NEW_ALIGNMENT__;
 #else
-  return __align > alignment_of::value;
+  return __align > alignment_of<__libcpp_max_align_t>::value;
 #endif
 }
 
Index: libcxx/include/cstddef
===
--- libcxx/include/cstddef
+++ libcxx/include/cstddef
@@ -25,7 +25,7 @@
 
 ptrdiff_t
 size_t
-max_align_t
+max_align_t // C++11
 nullptr_t
 byte // C++17
 
@@ -49,12 +49,8 @@
 using ::ptrdiff_t;
 using ::size_t;
 
-#if defined(__CLANG_MAX_ALIGN_T_DEFINED) || defined(_GCC_MAX_ALIGN_T) || \
-defined(__DEFINED_max_align_t) || defined(__NetBSD__)
-// Re-use the compiler's  max_align_t where possible.
+#if !defined(_LIBCPP_CXX03_LANG)
 using ::max_align_t;
-#else
-typedef long double max_align_t;
 #endif
 
 _LIBCPP_END_NAMESPACE_STD
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73245: Don't define std::max_align_t if not used in C++03 mode

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Let me clarify the situation for a moment:
(1) libc++ does try to work in C++03 mode. See the separate implementation of 
 for pre-C++11. It is also desirable to support. This is completely 
beside the question of TR1 support.
(2) The only reason why max_align_t is currently necessary is because it is 
used as default alignment in .
(3) Most compilers we care about already have a preprocessor symbol 
specifically for that purpose.

If (3) is present, we shouldn't pollute the global namespace, especially with a 
potentially bogus value. Ideally, this shouldn't be necessary at all, since 
overaligned new doesn't exist in C++03. But that would be a much more intrusive 
change to  and go beyond "try not to break c++03".


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

https://reviews.llvm.org/D73245



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

[PATCH] D73245: Don't define std::max_align_t if not used in C++03 mode

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

libc++ generally tries to play nice with C++03 code. It doesn't go out of its 
way to break it and keeping it usable helps dealing with a lot of rusty old 
code. That's what the patch is all about, not breaking things for no good 
reason.


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

https://reviews.llvm.org/D73245



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

[PATCH] D73245: Don't define std::max_align_t if not used in C++03 mode

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg created this revision.
joerg added a reviewer: mclow.lists.
Herald added a subscriber: christof.

max_align_t has been introduced by C++11 and C99. When an older language mode 
is explicitly requested, the system headers might not provide. Don't define it 
in this case unless other headers () depend on it.


https://reviews.llvm.org/D73245

Files:
  libcxx/include/cstddef


Index: libcxx/include/cstddef
===
--- libcxx/include/cstddef
+++ libcxx/include/cstddef
@@ -49,6 +49,9 @@
 using ::ptrdiff_t;
 using ::size_t;
 
+// max_align_t is part of C99/C++11, but necessary for 
+// if the compiler doesn't tell us the default new alignment.
+#if !defined(_LIBCPP_CXX03_LANG) || !defined(__STDCPP_DEFAULT_NEW_ALIGNMENT__)
 #if defined(__CLANG_MAX_ALIGN_T_DEFINED) || defined(_GCC_MAX_ALIGN_T) || \
 defined(__DEFINED_max_align_t) || defined(__NetBSD__)
 // Re-use the compiler's  max_align_t where possible.
@@ -56,6 +59,7 @@
 #else
 typedef long double max_align_t;
 #endif
+#endif
 
 _LIBCPP_END_NAMESPACE_STD
 


Index: libcxx/include/cstddef
===
--- libcxx/include/cstddef
+++ libcxx/include/cstddef
@@ -49,6 +49,9 @@
 using ::ptrdiff_t;
 using ::size_t;
 
+// max_align_t is part of C99/C++11, but necessary for 
+// if the compiler doesn't tell us the default new alignment.
+#if !defined(_LIBCPP_CXX03_LANG) || !defined(__STDCPP_DEFAULT_NEW_ALIGNMENT__)
 #if defined(__CLANG_MAX_ALIGN_T_DEFINED) || defined(_GCC_MAX_ALIGN_T) || \
 defined(__DEFINED_max_align_t) || defined(__NetBSD__)
 // Re-use the compiler's  max_align_t where possible.
@@ -56,6 +59,7 @@
 #else
 typedef long double max_align_t;
 #endif
+#endif
 
 _LIBCPP_END_NAMESPACE_STD
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72869: Add __warn_memset_zero_len builtin as a workaround for glibc issue

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Can this be restricted to Linux?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72869



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

[PATCH] D69825: [Clang][Driver] Re-use the calling process instead of creating a new process for the cc1 invocation

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added inline comments.



Comment at: clang/test/Driver/cc1-spawnprocess.c:1
+// RUN: env -u CLANG_SPAWN_CC1 %clang -c %s -o /dev/null
+// RUN: env CLANG_SPAWN_CC1=0 %clang -c %s -o /dev/null

mgorny wrote:
> `env -u` is not portable.
I think just going for `env CLANG_SPAWN_CC1=` works for the purpose of this 
test. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



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

[PATCH] D70416: [Driver] Make -static-libgcc imply static libunwind

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

This is normally done by using `-Bstatic`/`-Bdynamic` around the library. See 
`tools::addOpenMPRuntime`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70416



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

[PATCH] D67638: Improve __builtin_constant_p lowering

[Joerg Sonnenberger via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG529f4ed401ea: Improve __builtin_constant_p lowering 
(authored by joerg).

Changed prior to commit:
  https://reviews.llvm.org/D67638?vs=220398&id=224796#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67638

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/builtin-constant-p.c
  clang/test/CodeGen/ppc-emmintrin.c
  clang/test/CodeGen/ppc-tmmintrin.c

Index: clang/test/CodeGen/ppc-tmmintrin.c
===
--- clang/test/CodeGen/ppc-tmmintrin.c
+++ clang/test/CodeGen/ppc-tmmintrin.c
@@ -108,7 +108,8 @@
 // CHECK: store <2 x i64> [[REG61]], <2 x i64>* [[REG65:[0-9a-zA-Z_%.]+]], align 16
 // CHECK-NEXT: store <2 x i64> [[REG62]], <2 x i64>* [[REG66:[0-9a-zA-Z_%.]+]], align 16
 // CHECK-NEXT: store i32 [[REG63]], i32* [[REG64:[0-9a-zA-Z_%.]+]], align 4
-// CHECK-NEXT: br i1 false, label %[[REG67:[0-9a-zA-Z_%.]+]], label %[[REG68:[0-9a-zA-Z_%.]+]]
+// CHECK: %[[REG64b:[0-9a-zA-Z_%.]+]] = call i1 @llvm.is.constant.i32(i32 %0)
+// CHECK-NEXT: br i1 %[[REG64b]], label %[[REG67:[0-9a-zA-Z_%.]+]], label %[[REG68:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG67]]:
 // CHECK-NEXT: [[REG69:[0-9a-zA-Z_%.]+]] = load i32, i32* [[REG64]], align 4
Index: clang/test/CodeGen/ppc-emmintrin.c
===
--- clang/test/CodeGen/ppc-emmintrin.c
+++ clang/test/CodeGen/ppc-emmintrin.c
@@ -231,7 +231,9 @@
 // CHECK-NEXT: br i1 [[REG147]], label %[[REG148:[0-9a-zA-Z_%.]+]], label %[[REG149:[0-9a-zA-Z_%.]+]]
 // CHECK: [[REG148]]:
 
-// CHECK-LE-NEXT: br i1 false, label %[[REG150:[0-9a-zA-Z_%.]+]], label %[[REG151:[0-9a-zA-Z_%.]+]]
+// CHECK-LE-NEXT: load
+// CHECK-LE-NEXT: call i1 @llvm.is.constant
+// CHECK-LE-NEXT: br i1 %[[REG1896a:[0-9a-zA-Z_%.]+]], label %[[REG150:[0-9a-zA-Z_%.]+]], label %[[REG151:[0-9a-zA-Z_%.]+]]
 // CHECK-LE: [[REG150]]:
 // CHECK-LE: [[REG152:[0-9a-zA-Z_%.]+]] = load <2 x i64>, <2 x i64>* [[REG143]], align 16
 // CHECK-LE-NEXT: [[REG153:[0-9a-zA-Z_%.]+]] = bitcast <2 x i64> [[REG152]] to <16 x i8>
@@ -2326,7 +2328,9 @@
 // CHECK-NEXT: br i1 [[REG1559]], label %[[REG1560:[0-9a-zA-Z_%.]+]], label %[[REG1557:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1560]]:
-// CHECK-NEXT: br i1 false, label %[[REG1561:[0-9a-zA-Z_%.]+]], label %[[REG1562:[0-9a-zA-Z_%.]+]]
+// CHECK-NEXT: load
+// CHECK-NEXT: call i1 @llvm.is.constant
+// CHECK-NEXT: br i1 %[[REG1561a:[0-9a-zA-Z_%.]+]], label %[[REG1561:[0-9a-zA-Z_%.]+]], label %[[REG1562:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1561]]:
 // CHECK-NEXT: [[REG1563:[0-9a-zA-Z_%.]+]] = load i32, i32* [[REG1552]], align 4
@@ -2369,7 +2373,9 @@
 // CHECK-NEXT: br i1 [[REG1587]], label %[[REG1588:[0-9a-zA-Z_%.]+]], label %[[REG1585:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1588]]:
-// CHECK-NEXT: br i1 false, label %[[REG1589:[0-9a-zA-Z_%.]+]], label %[[REG1590:[0-9a-zA-Z_%.]+]]
+// CHECK-NEXT: load
+// CHECK-NEXT: call i1 @llvm.is.constant
+// CHECK-NEXT: br i1 %{{[0-9a-zA-Z_%.]+}}, label %[[REG1589:[0-9a-zA-Z_%.]+]], label %[[REG1590:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1589]]:
 // CHECK-NEXT: [[REG1591:[0-9a-zA-Z_%.]+]] = load i32, i32* [[REG1580]], align 4
@@ -2416,7 +2422,9 @@
 // CHECK-NEXT: br i1 [[REG1617]], label %[[REG1618:[0-9a-zA-Z_%.]+]], label %[[REG1615:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1618]]:
-// CHECK-NEXT: br i1 false, label %[[REG1619:[0-9a-zA-Z_%.]+]], label %[[REG1620:[0-9a-zA-Z_%.]+]]
+// CHECK-NEXT: load
+// CHECK-NEXT: call i1 @llvm.is.constant
+// CHECK-NEXT: br i1 %{{[0-9a-zA-Z_%.]+}}, label %[[REG1619:[0-9a-zA-Z_%.]+]], label %[[REG1620:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1619]]:
 // CHECK-NEXT: [[REG1621:[0-9a-zA-Z_%.]+]] = load i32, i32* [[REG1610]], align 4
@@ -2563,7 +2571,9 @@
 // CHECK-NEXT: br i1 [[REG1712]], label %[[REG1713:[0-9a-zA-Z_%.]+]], label %[[REG1714:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1713]]:
-// CHECK-NEXT: br i1 false, label %[[REG1715:[0-9a-zA-Z_%.]+]], label %[[REG1716:[0-9a-zA-Z_%.]+]]
+// CHECK-NEXT: load
+// CHECK-NEXT: call i1 @llvm.is.constant
+// CHECK-NEXT: br i1 %[[REG1715a:[0-9a-zA-Z_%.]+]], label %[[REG1715:[0-9a-zA-Z_%.]+]], label %[[REG1716:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1715]]:
 // CHECK-NEXT: [[REG1717:[0-9a-zA-Z_%.]+]] = load i32, i32* [[REG1709]], align 4
@@ -2601,7 +2611,9 @@
 // CHECK-NEXT: br i1 [[REG1737]], label %[[REG1738:[0-9a-zA-Z_%.]+]], label %[[REG1739:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1738]]:
-// CHECK-NEXT: br i1 false, label %[[REG1740:[0-9a-zA-Z_%.]+]], label %[[REG1741:[0-9a-zA-Z_%.]+]]
+// CHECK-NEXT: load
+// CHECK-NEXT: call i1 @llvm.is.constant
+// CHECK-NEXT: br i1 %[[REG1738:[0-9a-zA-Z_%.]+]], label %[[REG1740:[0-9a-zA-Z_%.]+]], label %[[REG1741:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1740]]:
 // CHECK-NEXT: [[REG1742:[0-9a-zA-Z_%.]+]] = load i32, i32* [[REG1734]], align 4
@@ -2741,7 +2753,9 @@
 // CHECK-NEXT: [[REG1837:

[PATCH] D68410: [AttrDocs] document always_inline

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

I wonder if we should actually enumerate evil here, i.e. give the situations in 
which inlining actually fails. As mentioned on IRC, I wonder if we shouldn't 
aim for the stronger semantics and at least warn by default of any situation 
that prevents always_inline from doing its job.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68410



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

[PATCH] D67638: Improve __builtin_constant_p lowering

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Ping.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67638



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

[PATCH] D67638: Improve __builtin_constant_p lowering

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg created this revision.
joerg added a reviewer: rsmith.
Herald added subscribers: kristina, kbarton, nemanjai.
Herald added a project: clang.
Herald added a subscriber: wuzish.

__builtin_constant_p used to be short-cut evaluated to false when building with 
-O0. This is undesirable as it means that constant folding in the front-end can 
give different results than folding in the back-end.  It can also create 
conditional branches on constant conditions that don't get folded away. With 
the improvements to the llvm.is.constant handling on the LLVM side, the 
short-cut is no longer useful.

Adjust various codegen tests to not depend on the optimizations.


Repository:
  rC Clang

https://reviews.llvm.org/D67638

Files:
  lib/CodeGen/CGBuiltin.cpp
  test/CodeGen/builtin-constant-p.c
  test/CodeGen/ppc-emmintrin.c
  test/CodeGen/ppc-tmmintrin.c

Index: test/CodeGen/ppc-tmmintrin.c
===
--- test/CodeGen/ppc-tmmintrin.c
+++ test/CodeGen/ppc-tmmintrin.c
@@ -108,7 +108,8 @@
 // CHECK: store <2 x i64> [[REG61]], <2 x i64>* [[REG65:[0-9a-zA-Z_%.]+]], align 16
 // CHECK-NEXT: store <2 x i64> [[REG62]], <2 x i64>* [[REG66:[0-9a-zA-Z_%.]+]], align 16
 // CHECK-NEXT: store i32 [[REG63]], i32* [[REG64:[0-9a-zA-Z_%.]+]], align 4
-// CHECK-NEXT: br i1 false, label %[[REG67:[0-9a-zA-Z_%.]+]], label %[[REG68:[0-9a-zA-Z_%.]+]]
+// CHECK: %[[REG64b:[0-9a-zA-Z_%.]+]] = call i1 @llvm.is.constant.i32(i32 %0)
+// CHECK-NEXT: br i1 %[[REG64b]], label %[[REG67:[0-9a-zA-Z_%.]+]], label %[[REG68:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG67]]:
 // CHECK-NEXT: [[REG69:[0-9a-zA-Z_%.]+]] = load i32, i32* [[REG64]], align 4
Index: test/CodeGen/ppc-emmintrin.c
===
--- test/CodeGen/ppc-emmintrin.c
+++ test/CodeGen/ppc-emmintrin.c
@@ -231,7 +231,9 @@
 // CHECK-NEXT: br i1 [[REG147]], label %[[REG148:[0-9a-zA-Z_%.]+]], label %[[REG149:[0-9a-zA-Z_%.]+]]
 // CHECK: [[REG148]]:
 
-// CHECK-LE-NEXT: br i1 false, label %[[REG150:[0-9a-zA-Z_%.]+]], label %[[REG151:[0-9a-zA-Z_%.]+]]
+// CHECK-LE-NEXT: load
+// CHECK-LE-NEXT: call i1 @llvm.is.constant
+// CHECK-LE-NEXT: br i1 %[[REG1896a:[0-9a-zA-Z_%.]+]], label %[[REG150:[0-9a-zA-Z_%.]+]], label %[[REG151:[0-9a-zA-Z_%.]+]]
 // CHECK-LE: [[REG150]]:
 // CHECK-LE: [[REG152:[0-9a-zA-Z_%.]+]] = load <2 x i64>, <2 x i64>* [[REG143]], align 16
 // CHECK-LE-NEXT: [[REG153:[0-9a-zA-Z_%.]+]] = bitcast <2 x i64> [[REG152]] to <16 x i8>
@@ -2326,7 +2328,9 @@
 // CHECK-NEXT: br i1 [[REG1559]], label %[[REG1560:[0-9a-zA-Z_%.]+]], label %[[REG1557:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1560]]:
-// CHECK-NEXT: br i1 false, label %[[REG1561:[0-9a-zA-Z_%.]+]], label %[[REG1562:[0-9a-zA-Z_%.]+]]
+// CHECK-NEXT: load
+// CHECK-NEXT: call i1 @llvm.is.constant
+// CHECK-NEXT: br i1 %[[REG1561a:[0-9a-zA-Z_%.]+]], label %[[REG1561:[0-9a-zA-Z_%.]+]], label %[[REG1562:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1561]]:
 // CHECK-NEXT: [[REG1563:[0-9a-zA-Z_%.]+]] = load i32, i32* [[REG1552]], align 4
@@ -2369,7 +2373,9 @@
 // CHECK-NEXT: br i1 [[REG1587]], label %[[REG1588:[0-9a-zA-Z_%.]+]], label %[[REG1585:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1588]]:
-// CHECK-NEXT: br i1 false, label %[[REG1589:[0-9a-zA-Z_%.]+]], label %[[REG1590:[0-9a-zA-Z_%.]+]]
+// CHECK-NEXT: load
+// CHECK-NEXT: call i1 @llvm.is.constant
+// CHECK-NEXT: br i1 %{{[0-9a-zA-Z_%.]+}}, label %[[REG1589:[0-9a-zA-Z_%.]+]], label %[[REG1590:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1589]]:
 // CHECK-NEXT: [[REG1591:[0-9a-zA-Z_%.]+]] = load i32, i32* [[REG1580]], align 4
@@ -2416,7 +2422,9 @@
 // CHECK-NEXT: br i1 [[REG1617]], label %[[REG1618:[0-9a-zA-Z_%.]+]], label %[[REG1615:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1618]]:
-// CHECK-NEXT: br i1 false, label %[[REG1619:[0-9a-zA-Z_%.]+]], label %[[REG1620:[0-9a-zA-Z_%.]+]]
+// CHECK-NEXT: load
+// CHECK-NEXT: call i1 @llvm.is.constant
+// CHECK-NEXT: br i1 %{{[0-9a-zA-Z_%.]+}}, label %[[REG1619:[0-9a-zA-Z_%.]+]], label %[[REG1620:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1619]]:
 // CHECK-NEXT: [[REG1621:[0-9a-zA-Z_%.]+]] = load i32, i32* [[REG1610]], align 4
@@ -2563,7 +2571,9 @@
 // CHECK-NEXT: br i1 [[REG1712]], label %[[REG1713:[0-9a-zA-Z_%.]+]], label %[[REG1714:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1713]]:
-// CHECK-NEXT: br i1 false, label %[[REG1715:[0-9a-zA-Z_%.]+]], label %[[REG1716:[0-9a-zA-Z_%.]+]]
+// CHECK-NEXT: load
+// CHECK-NEXT: call i1 @llvm.is.constant
+// CHECK-NEXT: br i1 %[[REG1715a:[0-9a-zA-Z_%.]+]], label %[[REG1715:[0-9a-zA-Z_%.]+]], label %[[REG1716:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1715]]:
 // CHECK-NEXT: [[REG1717:[0-9a-zA-Z_%.]+]] = load i32, i32* [[REG1709]], align 4
@@ -2601,7 +2611,9 @@
 // CHECK-NEXT: br i1 [[REG1737]], label %[[REG1738:[0-9a-zA-Z_%.]+]], label %[[REG1739:[0-9a-zA-Z_%.]+]]
 
 // CHECK: [[REG1738]]:
-// CHECK-NEXT: br i1 false, label %[[REG1740:[0-9a-zA-Z_%.]+]], label %[[REG1741:[0-9a-zA-Z_%.]+]]
+// CHECK-NEXT: load
+// CHECK-NEXT: call i1 @llvm.is.constant
+//

[PATCH] D60943: Delay diagnosing asm constraints that require immediates until after inlining

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

The combination of D60942 , D06943 and D65280 
 solves the problems for me on all targets I 
have.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60943



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

[PATCH] D60943: Delay diagnosing asm constraints that require immediates until after inlining

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

I understand, but the current version just doesn't work anyway to delay it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60943



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

[PATCH] D60943: Delay diagnosing asm constraints that require immediates until after inlining

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

You lost the changes to lib/Sema/SemaStmtAsm.cpp to actually do the delaying 
for immediate operands?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60943



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

[PATCH] D59744: Fix i386 ABI "__m64" type bug

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

I think MMX code is obscure enough at this point that it doesn't matter much 
either way. Even less across DSO boundaries. That's why I don't really care 
either way.


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

https://reviews.llvm.org/D59744



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

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added inline comments.



Comment at: lib/CodeGen/TargetInfo.cpp:1501
+//
+// Exclude other System V OS (e.g Darwin, PS4 and FreeBSD) since we don't
+// want to spend any effort dealing with the ramifications of ABI breaks.

krytarowski wrote:
> Darwin and BSD are not System V.
> 
> CC: @joerg @mgorny for NetBSD. Do we need to do something here?
It's a misnomer. The ABI standard for i386 was the SysV ABI before GNU decided 
to silently break the stack alignment and calling it the new ABI. That said, 
I'm not sure how much copy-by-value of vector types actually happens and that's 
the only situation affected by this.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60748



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

[PATCH] D60943: Delay diagnosing "n" constraint until after inlining

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

I'm in the process of testing this, but feedback will take a bit.

On the more meaty parts of this change, I think further iterations will be 
necessary in-tree to extend this to the other constraints.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60943



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

[PATCH] D59118: creduce script for clang crashes

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

For it to be really useful for the majority of bugs, it would be nice to figure 
out automatically how to get the preprocessing step done and filter out the # 
lines afterwards. That part alone significantly cuts down the creduce time.


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

https://reviews.llvm.org/D59118



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

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Well, that was a sample to illustrate the point. A full working (and now 
failing) example is:

  static inline void outl(unsigned port, unsigned data) {
if (__builtin_constant_p(port) && port < 0x100) {
  __asm volatile("outl %0,%w1" : : "a"(data), "n"(port));
} else {
  __asm volatile("outl %0,%w1" : : "a"(data), "d"(port));
}
  }
  
  void f(unsigned port) { outl(1, 1); }


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

https://reviews.llvm.org/D58821



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

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

The other problem is that we don't use the CFG machinery to prune dead 
branches. Consider the x86 in/out instructions: one variant takes an immediate, 
the other a register. The classic way to deal with that is something like

  static inline void outl(unsigned port, uint32_t value)
  {
if (__builtin_constant_p(port) && port < 0x100) {
  __asm volatile("outl %0,%w1" : : "a" (data), "id" (port));
} else {
 __asm volatile("outl %0,%w1" : : "a" (data), "d" (port));
}
  }

This fails with the new asm constraint checks, since the dead branch is never 
pruned. For other architectures it makes an even greater difference. The main 
reason it is a show stopper: there is no sane workaround that doesn't regress 
code quality.


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

https://reviews.llvm.org/D58821



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

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Can you include a patch for something like (int *)0xdeadbeef on amd64? 
That's a valid value for "n", but clearly too large for int. Thanks for looking 
at this, it is one of the two large remaining show stoppers for the asm 
constraint check.


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

https://reviews.llvm.org/D58821



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

[PATCH] D58649: Fix inline assembler constraint validation

[Joerg Sonnenberger via Phabricator via cfe-commits]

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



Comment at: include/clang/Basic/TargetInfo.h:860
+  if (!ImmSet.empty())
+return ImmSet.count(Value.getZExtValue()) != 0;
+  return !ImmRange.isConstrained || (Value.sge(ImmRange.Min) && 
Value.sle(ImmRange.Max));

efriedma wrote:
> Isn't the "ImmSet.count" call still wrong?  It looks like it crashes on an 
> 128-bit immediate, and implicitly truncates a 64-bit immediate.  I guess you 
> could check `Value.isSignedIntN(32)`?
I've added that and a test case for truncation as a follow-up.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58649



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

[PATCH] D58649: Fix inline assembler constraint validation

[Joerg Sonnenberger via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL354937: Fix inline assembler constraint validation (authored 
by joerg, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58649?vs=188265&id=188477#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58649

Files:
  cfe/trunk/include/clang/Basic/TargetInfo.h
  cfe/trunk/test/Sema/inline-asm-validate-x86.c


Index: cfe/trunk/include/clang/Basic/TargetInfo.h
===
--- cfe/trunk/include/clang/Basic/TargetInfo.h
+++ cfe/trunk/include/clang/Basic/TargetInfo.h
@@ -816,6 +816,7 @@
 struct {
   int Min;
   int Max;
+  bool isConstrained;
 } ImmRange;
 llvm::SmallSet ImmSet;
 
@@ -826,6 +827,7 @@
 : Flags(0), TiedOperand(-1), ConstraintStr(ConstraintStr.str()),
   Name(Name.str()) {
   ImmRange.Min = ImmRange.Max = 0;
+  ImmRange.isConstrained = false;
 }
 
 const std::string &getConstraintStr() const { return ConstraintStr; }
@@ -854,8 +856,9 @@
   return (Flags & CI_ImmediateConstant) != 0;
 }
 bool isValidAsmImmediate(const llvm::APInt &Value) const {
-  return (Value.sge(ImmRange.Min) && Value.sle(ImmRange.Max)) ||
- ImmSet.count(Value.getZExtValue()) != 0;
+  if (!ImmSet.empty())
+return ImmSet.count(Value.getZExtValue()) != 0;
+  return !ImmRange.isConstrained || (Value.sge(ImmRange.Min) && 
Value.sle(ImmRange.Max));
 }
 
 void setIsReadWrite() { Flags |= CI_ReadWrite; }
@@ -867,6 +870,7 @@
   Flags |= CI_ImmediateConstant;
   ImmRange.Min = Min;
   ImmRange.Max = Max;
+  ImmRange.isConstrained = true;
 }
 void setRequiresImmediate(llvm::ArrayRef Exacts) {
   Flags |= CI_ImmediateConstant;
@@ -879,8 +883,6 @@
 }
 void setRequiresImmediate() {
   Flags |= CI_ImmediateConstant;
-  ImmRange.Min = INT_MIN;
-  ImmRange.Max = INT_MAX;
 }
 
 /// Indicate that this is an input operand that is tied to
Index: cfe/trunk/test/Sema/inline-asm-validate-x86.c
===
--- cfe/trunk/test/Sema/inline-asm-validate-x86.c
+++ cfe/trunk/test/Sema/inline-asm-validate-x86.c
@@ -55,6 +55,7 @@
 void L(int i, int j) {
   static const int Invalid1 = 1;
   static const int Invalid2 = 42;
+  static const int Invalid3 = 0;
   static const int Valid1 = 0xff;
   static const int Valid2 = 0x;
   static const int Valid3 = 0x;
@@ -69,6 +70,9 @@
   : "0"(i), "L"(Invalid2)); // expected-error{{value '42' out of range 
for constraint 'L'}}
   __asm__("xorl %0,%2"
   : "=r"(i)
+  : "0"(i), "L"(Invalid3)); // expected-error{{value '0' out of range 
for constraint 'L'}}
+  __asm__("xorl %0,%2"
+  : "=r"(i)
   : "0"(i), "L"(Valid1)); // expected-no-error
   __asm__("xorl %0,%2"
   : "=r"(i)


Index: cfe/trunk/include/clang/Basic/TargetInfo.h
===
--- cfe/trunk/include/clang/Basic/TargetInfo.h
+++ cfe/trunk/include/clang/Basic/TargetInfo.h
@@ -816,6 +816,7 @@
 struct {
   int Min;
   int Max;
+  bool isConstrained;
 } ImmRange;
 llvm::SmallSet ImmSet;
 
@@ -826,6 +827,7 @@
 : Flags(0), TiedOperand(-1), ConstraintStr(ConstraintStr.str()),
   Name(Name.str()) {
   ImmRange.Min = ImmRange.Max = 0;
+  ImmRange.isConstrained = false;
 }
 
 const std::string &getConstraintStr() const { return ConstraintStr; }
@@ -854,8 +856,9 @@
   return (Flags & CI_ImmediateConstant) != 0;
 }
 bool isValidAsmImmediate(const llvm::APInt &Value) const {
-  return (Value.sge(ImmRange.Min) && Value.sle(ImmRange.Max)) ||
- ImmSet.count(Value.getZExtValue()) != 0;
+  if (!ImmSet.empty())
+return ImmSet.count(Value.getZExtValue()) != 0;
+  return !ImmRange.isConstrained || (Value.sge(ImmRange.Min) && Value.sle(ImmRange.Max));
 }
 
 void setIsReadWrite() { Flags |= CI_ReadWrite; }
@@ -867,6 +870,7 @@
   Flags |= CI_ImmediateConstant;
   ImmRange.Min = Min;
   ImmRange.Max = Max;
+  ImmRange.isConstrained = true;
 }
 void setRequiresImmediate(llvm::ArrayRef Exacts) {
   Flags |= CI_ImmediateConstant;
@@ -879,8 +883,6 @@
 }
 void setRequiresImmediate() {
   Flags |= CI_ImmediateConstant;
-  ImmRange.Min = INT_MIN;
-  ImmRange.Max = INT_MAX;
 }
 
 /// Indicate that this is an input operand that is tied to
Index: cfe/trunk/test/Sema/inline-asm-validate-x86.c
===
--- cfe/trunk/test/Sema/inline-asm-validate-x86.c
+++ cfe/trunk/test/Sema/inline-asm-validate-x86.c
@@ -55,6 +55,7 @@
 void L(int i, int j) {
   static c

[PATCH] D58649: Fix inline assembler constraint validation

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg created this revision.
joerg added a reviewer: compnerd.
Herald added a subscriber: eraman.

The current constraint logic is both too lax and too strict. It fails for input 
outside the [INT_MIN..INT_MAX] range, but it also implicitly accepts 0 as value 
when it should not. Adjust logic to handle both correctly.


https://reviews.llvm.org/D58649

Files:
  include/clang/Basic/TargetInfo.h
  test/Sema/inline-asm-validate-x86.c


Index: test/Sema/inline-asm-validate-x86.c
===
--- test/Sema/inline-asm-validate-x86.c
+++ test/Sema/inline-asm-validate-x86.c
@@ -55,6 +55,7 @@
 void L(int i, int j) {
   static const int Invalid1 = 1;
   static const int Invalid2 = 42;
+  static const int Invalid3 = 0;
   static const int Valid1 = 0xff;
   static const int Valid2 = 0x;
   static const int Valid3 = 0x;
@@ -69,6 +70,9 @@
   : "0"(i), "L"(Invalid2)); // expected-error{{value '42' out of range 
for constraint 'L'}}
   __asm__("xorl %0,%2"
   : "=r"(i)
+  : "0"(i), "L"(Invalid3)); // expected-error{{value '0' out of range 
for constraint 'L'}}
+  __asm__("xorl %0,%2"
+  : "=r"(i)
   : "0"(i), "L"(Valid1)); // expected-no-error
   __asm__("xorl %0,%2"
   : "=r"(i)
Index: include/clang/Basic/TargetInfo.h
===
--- include/clang/Basic/TargetInfo.h
+++ include/clang/Basic/TargetInfo.h
@@ -816,6 +816,7 @@
 struct {
   int Min;
   int Max;
+  bool isConstrained;
 } ImmRange;
 llvm::SmallSet ImmSet;
 
@@ -826,6 +827,7 @@
 : Flags(0), TiedOperand(-1), ConstraintStr(ConstraintStr.str()),
   Name(Name.str()) {
   ImmRange.Min = ImmRange.Max = 0;
+  ImmRange.isConstrained = false;
 }
 
 const std::string &getConstraintStr() const { return ConstraintStr; }
@@ -854,8 +856,9 @@
   return (Flags & CI_ImmediateConstant) != 0;
 }
 bool isValidAsmImmediate(const llvm::APInt &Value) const {
-  return (Value.sge(ImmRange.Min) && Value.sle(ImmRange.Max)) ||
- ImmSet.count(Value.getZExtValue()) != 0;
+  if (!ImmSet.empty())
+return ImmSet.count(Value.getZExtValue()) != 0;
+  return !ImmRange.isConstrained || (Value.sge(ImmRange.Min) && 
Value.sle(ImmRange.Max));
 }
 
 void setIsReadWrite() { Flags |= CI_ReadWrite; }
@@ -867,6 +870,7 @@
   Flags |= CI_ImmediateConstant;
   ImmRange.Min = Min;
   ImmRange.Max = Max;
+  ImmRange.isConstrained = true;
 }
 void setRequiresImmediate(llvm::ArrayRef Exacts) {
   Flags |= CI_ImmediateConstant;
@@ -879,8 +883,6 @@
 }
 void setRequiresImmediate() {
   Flags |= CI_ImmediateConstant;
-  ImmRange.Min = INT_MIN;
-  ImmRange.Max = INT_MAX;
 }
 
 /// Indicate that this is an input operand that is tied to


Index: test/Sema/inline-asm-validate-x86.c
===
--- test/Sema/inline-asm-validate-x86.c
+++ test/Sema/inline-asm-validate-x86.c
@@ -55,6 +55,7 @@
 void L(int i, int j) {
   static const int Invalid1 = 1;
   static const int Invalid2 = 42;
+  static const int Invalid3 = 0;
   static const int Valid1 = 0xff;
   static const int Valid2 = 0x;
   static const int Valid3 = 0x;
@@ -69,6 +70,9 @@
   : "0"(i), "L"(Invalid2)); // expected-error{{value '42' out of range for constraint 'L'}}
   __asm__("xorl %0,%2"
   : "=r"(i)
+  : "0"(i), "L"(Invalid3)); // expected-error{{value '0' out of range for constraint 'L'}}
+  __asm__("xorl %0,%2"
+  : "=r"(i)
   : "0"(i), "L"(Valid1)); // expected-no-error
   __asm__("xorl %0,%2"
   : "=r"(i)
Index: include/clang/Basic/TargetInfo.h
===
--- include/clang/Basic/TargetInfo.h
+++ include/clang/Basic/TargetInfo.h
@@ -816,6 +816,7 @@
 struct {
   int Min;
   int Max;
+  bool isConstrained;
 } ImmRange;
 llvm::SmallSet ImmSet;
 
@@ -826,6 +827,7 @@
 : Flags(0), TiedOperand(-1), ConstraintStr(ConstraintStr.str()),
   Name(Name.str()) {
   ImmRange.Min = ImmRange.Max = 0;
+  ImmRange.isConstrained = false;
 }
 
 const std::string &getConstraintStr() const { return ConstraintStr; }
@@ -854,8 +856,9 @@
   return (Flags & CI_ImmediateConstant) != 0;
 }
 bool isValidAsmImmediate(const llvm::APInt &Value) const {
-  return (Value.sge(ImmRange.Min) && Value.sle(ImmRange.Max)) ||
- ImmSet.count(Value.getZExtValue()) != 0;
+  if (!ImmSet.empty())
+return ImmSet.count(Value.getZExtValue()) != 0;
+  return !ImmRange.isConstrained || (Value.sge(ImmRange.Min) && Value.sle(ImmRange.Max));
 }
 
 void setIsReadWrite() { Flags |= CI_ReadWrite; }
@@ -867,6 +870,7 @@
   Flags |= CI_ImmediateConstant;
   ImmRange.Min = Min;
   ImmRange.Max = Max;
+  

[PATCH] D58592: [clang] [ToolChains/NetBSD] Support relative libc++ header path

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

I'm not in favor of this. It adds overhead for the system compiler and 
generally makes the logic more complicated. This seems to be another hack 
around the fact that the driver has no clear notion of "use system runtime" vs 
"use custom runtime".


Repository:
  rC Clang

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

https://reviews.llvm.org/D58592



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

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

As discussed with dankm on IRC, I still would like to see the correct behavior 
going into 8.0, i.e. not change it later. Since this also matters for potential 
faster implementations later, it seems like a good idea to do it now. The 
changes are well-localized.

(1) Do path prefix matching and not string prefix matching. The difference is 
that the file name must be longer than the prefix and the prefix must be 
followed by a path separator.
(2) The longest prefix match wins. Substituation is applied only once per file 
name, independent of the rules. This gives more predictable output and allows 
switching to a tree-lookup later.


Repository:
  rC Clang

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

https://reviews.llvm.org/D49466



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

[PATCH] D56647: [WIP] [ELF] Implement --copy-dt-needed-entries

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

As first step, it goes into the right direction. I would explicitly set 
--as-needed for all those indirectly loaded objects. If people want to retain 
the questionable behavior of newer GNU tools, it could be a separate flag so 
that a final round can warn if an indirectly pulled library is necessary, but 
that behavior doesn't IMO make much sense. Full version has to look at 
DT_RUNPATH/DT_RPATH and also --rpath-link.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56647



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

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Right, I'm not aware of anyone but FreeBSD really using the OSABI field. For 
FreeBSD it was a long standing hack around limitations in the ELF kernel 
loader. I'm not even sure if FreeBSD use(d) to set the OSABI field for the 
intermediate object files.


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

https://reviews.llvm.org/D56215



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

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

That's the other reason why I find the GCC specification as string prefix 
confusing. I still say we should just go with mapping of path names and then 
the order question mostly goes away.


Repository:
  rC Clang

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

https://reviews.llvm.org/D49466



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

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added inline comments.



Comment at: ELF/Driver.cpp:779
+// TODO: verify the triple somehow?
+Config->TargetTriple = llvm::Triple(Prefix);
+  }

mgorny wrote:
> joerg wrote:
> > See ToolChain::getTargetAndModeFromProgramName in clang.
> Yes, I've based this on stripped down version of that. Most notably, I wasn't 
> able to verify triple against TargetRegistry since it isn't initialized here.
initLLVM is called too late. That should be all that is needed to do a proper 
look up.


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

https://reviews.llvm.org/D56215



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

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added inline comments.



Comment at: ELF/Driver.cpp:381
+}
+Config->SearchPaths.push_back("/usr/lib");
+  }

Those need to be sysroot-relative of course.


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

https://reviews.llvm.org/D56215



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

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

@ruiu: No, it is exactly what you want, since it allows you to point lld into 
the normal sysroot. Cross-compiling is the default case for the NetBSD 
toolchain.


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

https://reviews.llvm.org/D56215



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

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added inline comments.



Comment at: ELF/Driver.cpp:779
+// TODO: verify the triple somehow?
+Config->TargetTriple = llvm::Triple(Prefix);
+  }

See ToolChain::getTargetAndModeFromProgramName in clang.


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

https://reviews.llvm.org/D56215



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