[PATCH] D50945: [Lex] Make HeaderMaps a unique_ptr vector

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Given the context (class an file name itself) and documentation around the 
function, I don't think in this particular case it improves readability or 
maintainability, the lifetime of the `HeaderMap` is (IMHO) fairly obvious from 
the const qualifier and from the documentation of the function itself. I would 
say leave it as is.


Repository:
  rC Clang

https://reviews.llvm.org/D50945



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

[PATCH] D50413: [libunwind][include] Add some missing definitions to .

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

This doesn't seem like a great idea to me, it's a negligible amount of type 
safety gained over using `stdint.h` types most people are more familiar with 
anyway. If you need something niche like those typedefs it's probably better to 
include the header and define them yourself in your application.


Repository:
  rUNW libunwind

https://reviews.llvm.org/D50413



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

[PATCH] D50564: Add support for SEH unwinding on Windows.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

I'm all for this change except the core issue is that you're using libunwind as 
a shim around the actual unwinding API provided by Windows. It would be nice to 
have something that did not have to do that and was capable of performing 
unwinding of SEH-style exceptions without needing additional runtime support.


Repository:
  rUNW libunwind

https://reviews.llvm.org/D50564



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

[PATCH] D50564: Add support for SEH unwinding on Windows.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

In https://reviews.llvm.org/D50564#1206393, @mstorsjo wrote:

> In https://reviews.llvm.org/D50564#1206370, @cdavis5x wrote:
>
> > In https://reviews.llvm.org/D50564#1206302, @kristina wrote:
> >
> > > I'm all for this change except the core issue is that you're using 
> > > libunwind as a shim around the actual unwinding API provided by Windows. 
> > > It would be nice to have something that did not have to do that and was 
> > > capable of performing unwinding of SEH-style exceptions without needing 
> > > additional runtime support.
> >
> >
> > It would be nice, but that would require extra work. We'd have to implement 
> > reading and interpreting unwind codes, and calling any handlers present at 
> > each frame (which all have a different calling convention from Itanium 
> > handlers), and handling chained unwind info... Or we could use the 
> > implementation that MS provided to us for free--and which gets loaded into 
> > every process anyway by virtue of being in NTDLL, and which is extremely 
> > well tested. Given all that, I'm wondering what implementing all that 
> > ourselves would gain us. I suppose we could eventually do all that, but for 
> > now, I think this is outside the scope of my change.
>
>
> +1. I guess such a library would be very nice to have, but from the point of 
> view of implementing exception handling, using the underlying APIs probably 
> is the way to go. The other question, as posted before, is whether we want to 
> wrap the whole CONTEXT structs in the UnwindCursor class and expose it via 
> the unw_* set of APIs. It gives quite a significant amount of extra code here 
> compared to libgcc's unwind-seh.c which is <500 loc altogether, providing 
> only the _Unwind_* API, implementing it directly with the Windows Rtl* APIs 
> from ntdll. That would give only a partial libunwind, with only the higher 
> level API available, but that's the only part used for exception handling at 
> least.


That still makes the underlying assumption that SEH is exclusive to Windows 
(and by that virtue PE/COFF) which doesn't necessarily hold true. There is also 
the issue of generic names like `_LIBUNWIND_SUPPORT_SEH_UNWIND` to guard 
includes of Windows specific headers which ties into my previous point. Would 
it be possible to somehow abstract away the Windows runtime function call and 
Windows specific header includes, even if it's via CMake options.

I'm raising this issue as there are other implementations of SEH that share the 
same (or extremely similar, depending on SDK versions) structures, but would 
not have the Windows headers available when compiling libunwind, and while the 
runtime function call to the `Rtl*` API is easy to change later on, separating 
rest of it from Windows headers is slightly messier.

Would it, at very least, be possible to decouple most of it from a dependency 
on Windows headers?

Would be much appreciated, as I'm going through a backlog of patches I'd like 
to put up for review one of which includes SEH for x86_64 Linux (ELF) 
implemented using RT signals and additional compiler runtime glue code, 
currently residing within compiler-rt builtins (one of the issues I want to 
address before). It's quite a wonky ABI since I tried to keep as much 
compatible with 64-bit Windows SEH (`.xdata` variation) in terms of frame 
lowering etc, but run-time mechanisms differ vastly for obvious reasons.


Repository:
  rUNW libunwind

https://reviews.llvm.org/D50564



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

[PATCH] D50564: Add support for SEH unwinding on Windows.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

In https://reviews.llvm.org/D50564#1208169, @cdavis5x wrote:

> OK, I've removed some of the dependencies on Windows-specific types. The code 
> in `Unwind-seh.cpp` is very Windows-specific, though, so I left it alone.


Much appreciated, thank you for your work. LGTM in terms of semantics.


Repository:
  rUNW libunwind

https://reviews.llvm.org/D50564



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

[PATCH] D50564: Add support for SEH unwinding on Windows.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added inline comments.



Comment at: src/UnwindCursor.hpp:1801
+  if (pc != getLastPC()) {
+UNWIND_INFO *xdata = reinterpret_cast(base + 
unwindEntry->UnwindData);
+if (xdata->Flags & (UNW_FLAG_EHANDLER|UNW_FLAG_UHANDLER)) {

mstorsjo wrote:
> I can't say I understand all of this yet, but I'm slowly getting there, and 
> I'm trying to compare this to what libgcc does.
> 
> libgcc doesn't use any definition of UNWIND_INFO and doesn't need to do the 
> equivalent of `getInfoFromSEH`, used by `step()`, anywhere. `unw_step()` is 
> used in `_Unwind_ForcedUnwind`, which in libgcc is implemented using 
> `RaiseException (STATUS_GCC_FORCED, ...`.
> 
> I guess if you happen to have all of the `unw_step` API available, it's 
> easier to just do it like this, in custom code without relying on the NTDLL 
> functions for it, while the libgcc version relies more on the NTDLL API.
This primarily deals with the SEH exceptions re-purposed as a C++ exception 
mechanism on x86_64 (if I understood this right), it's possible to set a custom 
filter using a runtime call so I suspect GCC does that or defines a translation 
function (also via a runtime call) which acts as a filter for "true" SEH 
exceptions behind the scenes deep within the runtime. Typially "true" SEH 
exceptions don't, outside of runtime magic, play nicely with C++ exceptions, 
with the `__C_specific_handler` ones being a completely different paradigm that 
falls far outside the scope of libunwind (those ones being the "true"/explicit 
SEH exceptions).

(Don't take my word for it, it's been a while and I only implemented the "true" 
variation for 64-bit Linux by reserving some RT signals and using that to 
invoke additional runtime glue that would then do the unwinding, completely 
ignoring DWARF since CFI exceptions and SEH exceptions really don't mix 
especially on platforms that are not Windows-like)


Repository:
  rUNW libunwind

https://reviews.llvm.org/D50564



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

[PATCH] D50564: Add support for SEH unwinding on Windows.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

I'd say LGTM since it's an introduction of any sort of **runtime** within the 
LLVM project scope that deals with SEH specifically. So far all the published 
code is pretty much exclusively related to Clang/LLVM IR and MC support for 
codegen of SEH related code, but with an implicit reliance on core Windows 
runtime and other goodies being around. Even if not super polished, as long as 
it does not cause regressions in other parts of libunwind, it's a start 
nontheless.


Repository:
  rUNW libunwind

https://reviews.llvm.org/D50564



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

[PATCH] D50413: [libunwind][include] Add some missing definitions to .

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Probably not, I'm personally against adding more and more typedefs for things 
that are just `stdint/inttypes.h` stuff and I say here the change is 
insignificant enough to warrant having to define even more types. It's not 
causing any problems but unless there are actual custom composite types that 
belong in that header, I don't see the benefit of them, if everyone starts 
wanting to typedef trivial types in `unwind.h`, it could be a maintainability 
issue. Not talking about this patch specifically but about a precedent of 
whether trivial types should get more and more aliases, polluting the header, 
and whether sticking to standard types and aliasing them in implementation is 
perhaps a better approach. Since the header gets installed everywhere and there 
isn't something like `unifdef` removing otherwise dead code. SEH patch was of 
much bigger significance so in that case it was fine (though I still like 
insisting on avoiding custom typedefs like `DWORD` for `uint32_t` just because 
rest of LLVM follows the same practice, while new composite types can be a 
necessity, more and more trivial type aliases isn't).

That's my stance on it. Patch itself is sound however, ignoring what I said 
above.


Repository:
  rUNW libunwind

https://reviews.llvm.org/D50413



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

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

LGTM.

Would allow for less repetition, ie. `always_inline` is an excellent candidate 
for cases like small functions that act as wrappers around bits of context 
sensitive inline asm and are usually clustered together in a header, a lot of 
embedded use cases come to mind, same goes for `alias`. Considering that 
everyone may have their own niche use cases for attributes, I think that's a 
very convenient change.


Repository:
  rC Clang

https://reviews.llvm.org/D51507



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

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added inline comments.



Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:58
 // CHECK-NEXT: IFunc (SubjectMatchRule_function)
+// CHECK-NEXT: InitPriority (SubjectMatchRule_variable)
 // CHECK-NEXT: InternalLinkage (SubjectMatchRule_variable, 
SubjectMatchRule_function, SubjectMatchRule_record)

Adding remark about `init_priority` as an inline comment. Wouldn't make sense 
semantically as it defeats the point of explicitly specifying static init order 
and cause a clash instead and causes ordering to become undefined again (as 
well as issuing a diagnostic I think).


Repository:
  rC Clang

https://reviews.llvm.org/D51507



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

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added inline comments.



Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:100
+// CHECK-NEXT: ObjCReturnsInnerPointer (SubjectMatchRule_objc_method, 
SubjectMatchRule_objc_property)
+// CHECK-NEXT: ObjCRootClass (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: ObjCRuntimeName (SubjectMatchRule_objc_interface, 
SubjectMatchRule_objc_protocol)

There is only one root class in a given runtime (ie. NSObject for objc4, or 
Object for older Apple runtimes, ObjFW also has a different root class). Having 
more than one makes no sense especially in the same lexical context as there 
are no use cases I can think of where you would have more than one active ObjC 
runtime within a process.


Repository:
  rC Clang

https://reviews.llvm.org/D51507



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

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added inline comments.



Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:100
+// CHECK-NEXT: ObjCReturnsInnerPointer (SubjectMatchRule_objc_method, 
SubjectMatchRule_objc_property)
+// CHECK-NEXT: ObjCRootClass (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: ObjCRuntimeName (SubjectMatchRule_objc_interface, 
SubjectMatchRule_objc_protocol)

rsmith wrote:
> kristina wrote:
> > There is only one root class in a given runtime (ie. NSObject for objc4, or 
> > Object for older Apple runtimes, ObjFW also has a different root class). 
> > Having more than one makes no sense especially in the same lexical context 
> > as there are no use cases I can think of where you would have more than one 
> > active ObjC runtime within a process.
> Thanks. Yes, this attribute probably doesn't make very much sense to use in 
> conjunction with the pragma.
> 
> So, do we explicitly disallow it, or do we allow it with the expectation that 
> it's likely that no-one will ever want to use it? (That is, do we want to 
> disallow cases that are not useful, or merely cases that are not meaningful?) 
> I don't have a strong opinion here, but I'm mildly inclined towards allowing 
> the pragma on any attribute where it's meaningful, as there may be useful 
> uses that we just didn't think of, and it costs nothing to permit.
Yes in theory it could only be used on a single interface in which case it 
would be perfectly valid. Otherwise when used incorrectly it would issue a 
diagnostic. As per your inclination of allowing it for all attributes I would 
say leave it allowed, as it **can** be used in a correct way. Diagnostics are 
sufficient enough to point out when it happens to apply the attribute to more 
than one interface.

So given your comment, I would say leave it as allowed.


Repository:
  rC Clang

https://reviews.llvm.org/D51507



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

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Giving it a second glance, just as an idea, maybe it's better to leave ObjC 
pragmas out for now, for another more narrower-scope revision/review specific 
to ObjC(/Swift)? Since they could cause incorrect code generation if combined 
in odd ways (as well as making no sense) especially with Automatic Reference 
Counting. For example a function annotated as returning a retained object while 
at the same time returning with release, and also a bridge annotation, may just 
miss the sema check and fail at runtime instead, which could be fairly erratic 
as it may vary depending on the `autoreleasepool` state as well as the runtime 
being used and could turn into a difficult bug to debug if it ever happened in 
the wild. I think semantic checker should catch the non-ObjC cases but only 
some ObjC cases. Another concern is Swift with Apple runtime and ObjC <=> Swift 
bridged objects (when Swift classes inherit ObjC classes or even interact with 
them) may introduce even more complications, and Swift patches and integration 
doesn't happen as part of mainline LLVM development, which complicates code 
review. Rest of it looks good however, not sure what you want to to do with the 
ObjC pragmas, they could be left in and re-reviewed later since that would mean 
they would end up in Swift development integration tree too.


Repository:
  rC Clang

https://reviews.llvm.org/D51507



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

[PATCH] D50246: [RISCV] Add support for computing sysroot for riscv32-unknown-elf

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Going to land it as it's approved by code owner.


Repository:
  rC Clang

https://reviews.llvm.org/D50246



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

[PATCH] D50246: [RISCV] Add support for computing sysroot for riscv32-unknown-elf

[Kristina Brooks via Phabricator via cfe-commits]

kristina closed this revision.
kristina added a comment.

Closed by https://reviews.llvm.org/rC341655.


Repository:
  rC Clang

https://reviews.llvm.org/D50246



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

[PATCH] D50246: [RISCV] Add support for computing sysroot for riscv32-unknown-elf

[Kristina Brooks via Phabricator via cfe-commits]

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

Seems to be causing a test failure for someone (per comment in 
https://reviews.llvm.org/rL341655), reopened it for now.


Repository:
  rC Clang

https://reviews.llvm.org/D50246



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

[PATCH] D50246: [RISCV] Add support for computing sysroot for riscv32-unknown-elf

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

In https://reviews.llvm.org/D50246#1229068, @rogfer01 wrote:

> Thanks for reopening this @kristina.
>
> I suggest passing `--sysroot=` to make sure we see the expected behaviour 
> when the sysroot is actually empty.
>
> Note that this would not really test the scenario where `DEFAULT_SYSROOT` is 
> empty **and** no `--sysroot` appears in the command line. I'm not sure if we 
> really want to test that case (but if we do, I think we will have to move 
> that case into a test of its own and add a //feature// in `lit.cfg.py` that 
> describes that clang does not have any built-in default sysroot).
>
> Thoughts?


Seems like a fairly niche case, you can request changes to this revision but 
there's been no regressions otherwise, buildbots have been handling this fine.

I think this may be something that would belong in a separate diff for the test 
suite, not for this particular diff. You can always just submit a diff for the 
test suite if you think it should have that option and if it would be useful as 
a general thing.


Repository:
  rC Clang

https://reviews.llvm.org/D50246



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

[PATCH] D50246: [RISCV] Add support for computing sysroot for riscv32-unknown-elf

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

In https://reviews.llvm.org/D50246#1229152, @rogfer01 wrote:

> Hi @kristina .
>
> Sure, I didn't mean to do that broader change here. Apologies if it read that 
> way.
>
> Would it be acceptable to add an empty `--sysroot=` to the test? I can post 
> the change for review in another diff.
>
> Thanks a lot.


Yes, you can submit another diff for this specific test, just use "Update Diff" 
and add yours on top which should open it up for re-review or use "Commandeer 
Revision" (and then submit another diff since it was previously closed). Up to 
your judgement.


Repository:
  rC Clang

https://reviews.llvm.org/D50246



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

[PATCH] D50246: [RISCV] Add support for computing sysroot for riscv32-unknown-elf

[Kristina Brooks via Phabricator via cfe-commits]

kristina closed this revision.
kristina added a comment.

In https://reviews.llvm.org/D50246#1229191, @lebedev.ri wrote:

> In https://reviews.llvm.org/D50246#1229177, @kristina wrote:
>
> > In https://reviews.llvm.org/D50246#1229152, @rogfer01 wrote:
> >
> > > Hi @kristina .
> > >
> > > Sure, I didn't mean to do that broader change here. Apologies if it read 
> > > that way.
> > >
> > > Would it be acceptable to add an empty `--sysroot=` to the test? I can 
> > > post the change for review in another diff.
> > >
> > > Thanks a lot.
> >
> >
> > Yes, you can submit another diff for this specific test, just use "Update 
> > Diff" and add yours on top which should open it up for re-review or use 
> > "Commandeer Revision" (and then submit another diff since it was previously 
> > closed). Up to your judgement.
>
>
> Please don't perform necromancy on already committed and closed differentials 
> (unless the commit was reverted, of course).
>  Do open new differentials.


Fair enough, sorry I suggested that.


Repository:
  rC Clang

https://reviews.llvm.org/D50246



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

[PATCH] D51899: Remove extraneous ".a" suffix from baremetal clang_rt.builtins when compiling for baremetal.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Please submit your diffs with context in the future (`-U9`). Also this 
seems like it would break a lot, `.a` is just a generic extension for an object 
archive file, I don't think the extension is the issue unless you're using some 
sort of an exotic linker.


https://reviews.llvm.org/D51899



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

[PATCH] D51899: Remove extraneous ".a" suffix from baremetal clang_rt.builtins when compiling for baremetal.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

The library shouldn't be an object file (it could be if you changed some stuff 
around but typically isn't), `libbuiltins` is a combination of multiple 
translation units, if you look at the source. I'm not sure how it's turning 
into an object file for you and even if it was you can pass an archive to the 
driver on the commandline and it would treat it like a group of objects. What 
is the generated output you're getting when you build it for bare metal?


https://reviews.llvm.org/D51899



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

[PATCH] D51899: Remove extraneous ".a" suffix from baremetal clang_rt.builtins when compiling for baremetal.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a reviewer: t.p.northover.
kristina added a comment.

Added the main ARM code owner as a reviewer, I'm not entirely sure why you're 
getting this behavior since regression tests should have weeded it out.


https://reviews.llvm.org/D51899



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

[PATCH] D51899: Remove extraneous ".a" suffix from baremetal clang_rt.builtins when compiling for baremetal.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Sorry, didn't notice this was recently added, from looking at Darwin and 
Android embedded tests, this doesn't really match up with either of them. The 
hosted tests for Android using `libbuiltins` look like this:

  // RUN: %clang -target arm-linux-androideabi -rtlib=compiler-rt -### %s 2>&1 
| FileCheck %s -check-prefix ARM-ANDROID
  // ARM-ANDROID: "{{.*[/\\]}}libclang_rt.builtins-arm-android.a"

Not sure what to make of this to be honest, I don't think this should be passed 
in as a conventional library since the driver usually picks the correct runtime 
for a given target triple.


https://reviews.llvm.org/D51899



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

[PATCH] D50359: Add a new library, libclang-cxx

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

I'm in support as long as it's a configuration option defaulting similar to 
LLVM's one. Should likely follow the same naming convention as LLVM, ie. 
`clang-shlib`. Clang has a lot of tools it ships with especially if you 
consider extras, I think this is one of the cases where an exception for 
`libClang-8.so` can be made as long as it's not a default. It would make builds 
of things like clang-tidy much faster without requiring a fully shared library 
build.


Repository:
  rC Clang

https://reviews.llvm.org/D50359



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

[PATCH] D50359: Add a new library, libclang-cxx

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

To elaborate, consider a scenario where a customer wants to build the clang 
driver, `diagtool` and just for the sake of the argument `change-namespace` 
from extras. Clang tools do not provide nearly the same level of control as 
most LLVM tools, so in that scenario, you'd have to maintain your own 
CMakeLists.txt for clang `tools(/extra)` to neuter unwanted artifacts like 
clang-tidy or libclang.so (the C API re-exporter) etc.

Unless I deeply misunderstand the functionality of toggles within LLVM's main 
build system, the only way of neutering those artifacts while still being able 
to use `libclang-cxx.so` would be surgery on several CMakeLists.


Repository:
  rC Clang

https://reviews.llvm.org/D50359



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

[PATCH] D51921: [VFS] vfs::directory_iterator yields path and file type instead of full Status

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Added inline comments regarding code style and internal/external naming.




Comment at: include/clang/Basic/VirtualFileSystem.h:135
+
+public:
+  directory_entry() = default;

The consistency is mostly related to naming of class members or function args 
or locals, I think the external APIs should use whatever is most suitable, in 
this case `path()` and `type()` and `status()` would make sense style wise, I 
think. Just my two cents in, not insisting on any style comments this time :)


Repository:
  rC Clang

https://reviews.llvm.org/D51921



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

[PATCH] D51921: [VFS] vfs::directory_iterator yields path and file type instead of full Status

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Fine with me, I wasn't really suggesting a revision just making a remark more 
or less. Sorry if it came off that way.


Repository:
  rC Clang

https://reviews.llvm.org/D51921



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

[PATCH] D52066: [Driver] Fix missing MultiArch include dir on powerpcspe

[Kristina Brooks via Phabricator via cfe-commits]

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

LGTM.

In the future please make sure you supply all patches with context (ie. 
`-U9`), without context they may be harder to review and more importantly 
`patch` may mess up.


Repository:
  rC Clang

https://reviews.llvm.org/D52066



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

[PATCH] D52066: [Driver] Fix missing MultiArch include dir on powerpcspe

[Kristina Brooks via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC342231: [Driver] Fix missing MultiArch include dir on 
powerpcspe (authored by kristina, committed by ).

Repository:
  rL LLVM

https://reviews.llvm.org/D52066

Files:
  lib/Driver/ToolChains/Linux.cpp


Index: lib/Driver/ToolChains/Linux.cpp
===
--- lib/Driver/ToolChains/Linux.cpp
+++ lib/Driver/ToolChains/Linux.cpp
@@ -699,7 +699,8 @@
   "/usr/include/mips64el-linux-gnu",
   "/usr/include/mips64el-linux-gnuabi64"};
   const StringRef PPCMultiarchIncludeDirs[] = {
-  "/usr/include/powerpc-linux-gnu"};
+  "/usr/include/powerpc-linux-gnu",
+  "/usr/include/powerpc-linux-gnuspe"};
   const StringRef PPC64MultiarchIncludeDirs[] = {
   "/usr/include/powerpc64-linux-gnu"};
   const StringRef PPC64LEMultiarchIncludeDirs[] = {


Index: lib/Driver/ToolChains/Linux.cpp
===
--- lib/Driver/ToolChains/Linux.cpp
+++ lib/Driver/ToolChains/Linux.cpp
@@ -699,7 +699,8 @@
   "/usr/include/mips64el-linux-gnu",
   "/usr/include/mips64el-linux-gnuabi64"};
   const StringRef PPCMultiarchIncludeDirs[] = {
-  "/usr/include/powerpc-linux-gnu"};
+  "/usr/include/powerpc-linux-gnu",
+  "/usr/include/powerpc-linux-gnuspe"};
   const StringRef PPC64MultiarchIncludeDirs[] = {
   "/usr/include/powerpc64-linux-gnu"};
   const StringRef PPC64LEMultiarchIncludeDirs[] = {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52066: [Driver] Fix missing MultiArch include dir on powerpcspe

[Kristina Brooks via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL342231: [Driver] Fix missing MultiArch include dir on 
powerpcspe (authored by kristina, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D52066?vs=165393&id=165477#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D52066

Files:
  cfe/trunk/lib/Driver/ToolChains/Linux.cpp


Index: cfe/trunk/lib/Driver/ToolChains/Linux.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Linux.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Linux.cpp
@@ -699,7 +699,8 @@
   "/usr/include/mips64el-linux-gnu",
   "/usr/include/mips64el-linux-gnuabi64"};
   const StringRef PPCMultiarchIncludeDirs[] = {
-  "/usr/include/powerpc-linux-gnu"};
+  "/usr/include/powerpc-linux-gnu",
+  "/usr/include/powerpc-linux-gnuspe"};
   const StringRef PPC64MultiarchIncludeDirs[] = {
   "/usr/include/powerpc64-linux-gnu"};
   const StringRef PPC64LEMultiarchIncludeDirs[] = {


Index: cfe/trunk/lib/Driver/ToolChains/Linux.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Linux.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Linux.cpp
@@ -699,7 +699,8 @@
   "/usr/include/mips64el-linux-gnu",
   "/usr/include/mips64el-linux-gnuabi64"};
   const StringRef PPCMultiarchIncludeDirs[] = {
-  "/usr/include/powerpc-linux-gnu"};
+  "/usr/include/powerpc-linux-gnu",
+  "/usr/include/powerpc-linux-gnuspe"};
   const StringRef PPC64MultiarchIncludeDirs[] = {
   "/usr/include/powerpc64-linux-gnu"};
   const StringRef PPC64LEMultiarchIncludeDirs[] = {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52093: [ToolChains] Linux.cpp: Use the same style across all all multi-arch includes. NFC.

[Kristina Brooks via Phabricator via cfe-commits]

kristina created this revision.
kristina added a reviewer: rsmith.
kristina added a project: clang.
Herald added a subscriber: cfe-commits.

Currently it seems like a mess in terms of where newlines are used. Cleaning it 
up so all of them use newlines and are aligned and moving the default switch 
case to the top. Making a differential to see if anyone has objections to the 
style used.


Repository:
  rC Clang

https://reviews.llvm.org/D52093

Files:
  lib/Driver/ToolChains/Linux.cpp


Index: lib/Driver/ToolChains/Linux.cpp
===
--- lib/Driver/ToolChains/Linux.cpp
+++ lib/Driver/ToolChains/Linux.cpp
@@ -671,14 +671,16 @@
   // FIXME: These are older forms of multiarch. It's not clear that they're
   // in use in any released version of Debian, so we should consider
   // removing them.
-  "/usr/include/i686-linux-gnu/64", "/usr/include/i486-linux-gnu/64"};
+  "/usr/include/i686-linux-gnu/64",
+  "/usr/include/i486-linux-gnu/64"};
   const StringRef X86MultiarchIncludeDirs[] = {
   "/usr/include/i386-linux-gnu",
 
   // FIXME: These are older forms of multiarch. It's not clear that they're
   // in use in any released version of Debian, so we should consider
   // removing them.
-  "/usr/include/x86_64-linux-gnu/32", "/usr/include/i686-linux-gnu",
+  "/usr/include/x86_64-linux-gnu/32",
+  "/usr/include/i686-linux-gnu",
   "/usr/include/i486-linux-gnu"};
   const StringRef AArch64MultiarchIncludeDirs[] = {
   "/usr/include/aarch64-linux-gnu"};
@@ -690,11 +692,13 @@
   "/usr/include/armeb-linux-gnueabi"};
   const StringRef ARMEBHFMultiarchIncludeDirs[] = {
   "/usr/include/armeb-linux-gnueabihf"};
-  const StringRef MIPSMultiarchIncludeDirs[] = {"/usr/include/mips-linux-gnu"};
+  const StringRef MIPSMultiarchIncludeDirs[] = {
+  "/usr/include/mips-linux-gnu"};
   const StringRef MIPSELMultiarchIncludeDirs[] = {
   "/usr/include/mipsel-linux-gnu"};
   const StringRef MIPS64MultiarchIncludeDirs[] = {
-  "/usr/include/mips64-linux-gnu", "/usr/include/mips64-linux-gnuabi64"};
+  "/usr/include/mips64-linux-gnu",
+  "/usr/include/mips64-linux-gnuabi64"};
   const StringRef MIPS64ELMultiarchIncludeDirs[] = {
   "/usr/include/mips64el-linux-gnu",
   "/usr/include/mips64el-linux-gnuabi64"};
@@ -713,6 +717,8 @@
   "/usr/include/s390x-linux-gnu"};
   ArrayRef MultiarchIncludeDirs;
   switch (getTriple().getArch()) {
+  default:
+break;
   case llvm::Triple::x86_64:
 MultiarchIncludeDirs = X86_64MultiarchIncludeDirs;
 break;
@@ -767,8 +773,6 @@
   case llvm::Triple::systemz:
 MultiarchIncludeDirs = SYSTEMZMultiarchIncludeDirs;
 break;
-  default:
-break;
   }
 
   const std::string AndroidMultiarchIncludeDir =


Index: lib/Driver/ToolChains/Linux.cpp
===
--- lib/Driver/ToolChains/Linux.cpp
+++ lib/Driver/ToolChains/Linux.cpp
@@ -671,14 +671,16 @@
   // FIXME: These are older forms of multiarch. It's not clear that they're
   // in use in any released version of Debian, so we should consider
   // removing them.
-  "/usr/include/i686-linux-gnu/64", "/usr/include/i486-linux-gnu/64"};
+  "/usr/include/i686-linux-gnu/64",
+  "/usr/include/i486-linux-gnu/64"};
   const StringRef X86MultiarchIncludeDirs[] = {
   "/usr/include/i386-linux-gnu",
 
   // FIXME: These are older forms of multiarch. It's not clear that they're
   // in use in any released version of Debian, so we should consider
   // removing them.
-  "/usr/include/x86_64-linux-gnu/32", "/usr/include/i686-linux-gnu",
+  "/usr/include/x86_64-linux-gnu/32",
+  "/usr/include/i686-linux-gnu",
   "/usr/include/i486-linux-gnu"};
   const StringRef AArch64MultiarchIncludeDirs[] = {
   "/usr/include/aarch64-linux-gnu"};
@@ -690,11 +692,13 @@
   "/usr/include/armeb-linux-gnueabi"};
   const StringRef ARMEBHFMultiarchIncludeDirs[] = {
   "/usr/include/armeb-linux-gnueabihf"};
-  const StringRef MIPSMultiarchIncludeDirs[] = {"/usr/include/mips-linux-gnu"};
+  const StringRef MIPSMultiarchIncludeDirs[] = {
+  "/usr/include/mips-linux-gnu"};
   const StringRef MIPSELMultiarchIncludeDirs[] = {
   "/usr/include/mipsel-linux-gnu"};
   const StringRef MIPS64MultiarchIncludeDirs[] = {
-  "/usr/include/mips64-linux-gnu", "/usr/include/mips64-linux-gnuabi64"};
+  "/usr/include/mips64-linux-gnu",
+  "/usr/include/mips64-linux-gnuabi64"};
   const StringRef MIPS64ELMultiarchIncludeDirs[] = {
   "/usr/include/mips64el-linux-gnu",
   "/usr/include/mips64el-linux-gnuabi64"};
@@ -713,6 +717,8 @@
   "/usr/include/s390x-linux-gnu"};
   ArrayRef MultiarchIncludeDirs;
   switch (getTriple().getArch()) {
+  default:
+break;
   case llvm::Triple::x86_64:
 MultiarchIncludeDirs = X86_64MultiarchIncludeDirs;
 break;
@@ -767,8 +773,6 @@
   ca

[PATCH] D43953: clangformat-vs: Fix plugin not correctly loading in some cases

[Kristina Brooks via Phabricator via cfe-commits]

kristina accepted this revision as: kristina.
kristina added a comment.
This revision is now accepted and ready to land.

Seems something that's not easy to test or reproduce even with VS (the plugin 
itself is 3rd party if I understand right) but it looks sane.


Repository:
  rC Clang

https://reviews.llvm.org/D43953



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

[PATCH] D53102: Support for the mno-tls-direct-seg-refs flag

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

@nruslan This patchset is still pending review so be patient, I landed 
https://reviews.llvm.org/D53103 as it was reviewed and accepted by the code 
owner, on which this patch depends on. That said it LGTM, it's simple enough as 
it just forwards the argument to the backend, but still would rather have the 
X86 code owner sign off on this as well.


https://reviews.llvm.org/D53102



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

[PATCH] D53102: Support for the mno-tls-direct-seg-refs flag

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

If the author doesn't mind I can just apply the style fix after patching and 
then rebuild and run all the relevant tests (or would you prefer to do that?). 
Seems easier than a new revision for an indentation change on one line.


https://reviews.llvm.org/D53102



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

[PATCH] D53102: Support for the mno-tls-direct-seg-refs flag

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

By the way, out of curiosity is this for anything specific (alternative libc or 
some user-mode-scheduling implementation)? Not nitpicking, just curious since 
it's an interesting topic in general and it's frustrating that the architecture 
is so limited in terms of registers that can be used for TLS/related data 
unlike newer ARM/ARM64 architectures.

Also FWIW `%gs` is generally free to use under x86_64 Linux which is where I 
usually place my thread control blocks which doesn't interfere with libc which 
uses `%fs`. (Just started build/test job, waiting on that for now)


https://reviews.llvm.org/D53102



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

[PATCH] D53102: Support for the mno-tls-direct-seg-refs flag

[Kristina Brooks via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC344739: Add support for -mno-tls-direct-seg-refs to Clang 
(authored by kristina, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D53102?vs=169224&id=170082#toc

Repository:
  rC Clang

https://reviews.llvm.org/D53102

Files:
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  lib/CodeGen/CGCall.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/indirect-tls-seg-refs.c
  test/Driver/indirect-tls-seg-refs.c


Index: include/clang/Frontend/CodeGenOptions.def
===
--- include/clang/Frontend/CodeGenOptions.def
+++ include/clang/Frontend/CodeGenOptions.def
@@ -62,6 +62,8 @@
 CODEGENOPT(DebugPassManager, 1, 0) ///< Prints debug information for the new
///< pass manager.
 CODEGENOPT(DisableRedZone, 1, 0) ///< Set when -mno-red-zone is enabled.
+CODEGENOPT(IndirectTlsSegRefs, 1, 0) ///< Set when -mno-tls-direct-seg-refs
+ ///< is specified.
 CODEGENOPT(DisableTailCalls  , 1, 0) ///< Do not emit tail calls.
 CODEGENOPT(NoEscapingBlockTailCalls, 1, 0) ///< Do not emit tail calls from
///< escaping blocks.
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -2011,6 +2011,8 @@
 def mno_pascal_strings : Flag<["-"], "mno-pascal-strings">,
   Alias;
 def mno_red_zone : Flag<["-"], "mno-red-zone">, Group;
+def mno_tls_direct_seg_refs : Flag<["-"], "mno-tls-direct-seg-refs">, 
Group, Flags<[CC1Option]>,
+  HelpText<"Disable direct TLS access through segment registers">;
 def mno_relax_all : Flag<["-"], "mno-relax-all">, Group;
 def mno_rtd: Flag<["-"], "mno-rtd">, Group;
 def mno_soft_float : Flag<["-"], "mno-soft-float">, Group;
@@ -2171,6 +2173,8 @@
 def moslib_EQ : Joined<["-"], "moslib=">, Group;
 def mpascal_strings : Flag<["-"], "mpascal-strings">, Alias;
 def mred_zone : Flag<["-"], "mred-zone">, Group;
+def mtls_direct_seg_refs : Flag<["-"], "mtls-direct-seg-refs">, Group,
+  HelpText<"Enable direct TLS access through segment registers (default)">;
 def mregparm_EQ : Joined<["-"], "mregparm=">, Group;
 def mrelax_all : Flag<["-"], "mrelax-all">, Group, 
Flags<[CC1Option,CC1AsOption]>,
   HelpText<"(integrated-as) Relax all machine instructions">;
Index: test/CodeGen/indirect-tls-seg-refs.c
===
--- test/CodeGen/indirect-tls-seg-refs.c
+++ test/CodeGen/indirect-tls-seg-refs.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 %s -emit-llvm -o - -mno-tls-direct-seg-refs | FileCheck %s 
-check-prefix=NO-TLSDIRECT
+// RUN: %clang_cc1 %s -emit-llvm -o - | FileCheck %s -check-prefix=TLSDIRECT
+
+// NO-TLSDIRECT: attributes #{{[0-9]+}} = {{{.*}} "indirect-tls-seg-refs"
+// TLSDIRECT-NOT: attributes #{{[0-9]+}} = {{{.*}} "indirect-tls-seg-refs"
+
+void test1() {
+}
Index: test/Driver/indirect-tls-seg-refs.c
===
--- test/Driver/indirect-tls-seg-refs.c
+++ test/Driver/indirect-tls-seg-refs.c
@@ -0,0 +1,7 @@
+// RUN: %clang -### %s 2>&1 | FileCheck %s -check-prefix=TLSDIRECT
+// RUN: %clang -### -mno-tls-direct-seg-refs -mtls-direct-seg-refs %s 2>&1 | 
FileCheck %s -check-prefix=TLSDIRECT
+// RUN: %clang -### -mtls-direct-seg-refs -mno-tls-direct-seg-refs %s 2>&1 | 
FileCheck %s -check-prefix=NO-TLSDIRECT
+// REQUIRES: clang-driver
+
+// NO-TLSDIRECT: -mno-tls-direct-seg-refs
+// TLSDIRECT-NOT: -mno-tls-direct-seg-refs
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -1709,6 +1709,8 @@
 
   if (CodeGenOpts.DisableRedZone)
 FuncAttrs.addAttribute(llvm::Attribute::NoRedZone);
+  if (CodeGenOpts.IndirectTlsSegRefs)
+FuncAttrs.addAttribute("indirect-tls-seg-refs");
   if (CodeGenOpts.NoImplicitFloat)
 FuncAttrs.addAttribute(llvm::Attribute::NoImplicitFloat);
 
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -1739,6 +1739,10 @@
   Args.hasArg(options::OPT_fapple_kext))
 CmdArgs.push_back("-disable-red-zone");
 
+  if (!Args.hasFlag(options::OPT_mtls_direct_seg_refs,
+options::OPT_mno_tls_direct_seg_refs, true))
+CmdArgs.push_back("-mno-tls-direct-seg-refs");
+
   // Default to avoid implicit floating-point for kernel/kext code, but allow
   // that to be overridden with -mno-soft-float.
   bool NoImplicitFloat = (Args.hasArg(options::OPT_mkernel) ||
Index: lib/Frontend/CompilerInvocation.cpp
==

[PATCH] D53475: Create ConstantExpr class

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

I think you may have accidentally commited the wrong patch.

  +struct ConstantExpr : public FullExpr {

Is causing a warning right now, not sure where that came from.


Repository:
  rC Clang

https://reviews.llvm.org/D53475



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

[PATCH] D53475: Create ConstantExpr class

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Ah, it was causing this warning during build:

  /SourceCache/llvm-trunk-8.0/tools/clang/include/clang/AST/Expr.h:903:1: 
warning: 'ConstantExpr' defined as a struct here but previously declared as a 
class [-Wmismatched-tags]


Repository:
  rC Clang

https://reviews.llvm.org/D53475



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

[PATCH] D56241: expose a control flag for interger to pointer ext warning

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Going to test and land it as requested by @lebedev.ri on IRC.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56241



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

[PATCH] D56241: [Sema] expose a control flag for integer to pointer ext warning

[Kristina Brooks via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC351082: [Sema] Expose a control flag for integer to pointer 
ext warning (authored by kristina, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D56241?vs=180143&id=181585#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D56241

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  test/Misc/warning-flags.c
  test/Sema/ext-typecheck-comparison-of-pointer-integer.c


Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5875,7 +5875,8 @@
 def err_typecheck_comparison_of_fptr_to_void : Error<
   "equality comparison between function pointer and void pointer (%0 and %1)">;
 def ext_typecheck_comparison_of_pointer_integer : ExtWarn<
-  "comparison between pointer and integer (%0 and %1)">;
+  "comparison between pointer and integer (%0 and %1)">,
+  InGroup>;
 def err_typecheck_comparison_of_pointer_integer : Error<
   "comparison between pointer and integer (%0 and %1)">;
 def ext_typecheck_comparison_of_distinct_pointers : ExtWarn<
Index: test/Misc/warning-flags.c
===
--- test/Misc/warning-flags.c
+++ test/Misc/warning-flags.c
@@ -18,7 +18,7 @@
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
-CHECK: Warnings without flags (75):
+CHECK: Warnings without flags (74):
 CHECK-NEXT:   ext_excess_initializers
 CHECK-NEXT:   ext_excess_initializers_in_char_array_initializer
 CHECK-NEXT:   ext_expected_semi_decl_list
@@ -29,7 +29,6 @@
 CHECK-NEXT:   ext_new_paren_array_nonconst
 CHECK-NEXT:   ext_plain_complex
 CHECK-NEXT:   ext_template_arg_extra_parens
-CHECK-NEXT:   ext_typecheck_comparison_of_pointer_integer
 CHECK-NEXT:   ext_typecheck_cond_incompatible_operands
 CHECK-NEXT:   ext_typecheck_ordered_comparison_of_pointer_integer
 CHECK-NEXT:   ext_using_undefined_std
Index: test/Sema/ext-typecheck-comparison-of-pointer-integer.c
===
--- test/Sema/ext-typecheck-comparison-of-pointer-integer.c
+++ test/Sema/ext-typecheck-comparison-of-pointer-integer.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fsyntax-only  -verify 
-DEXPECTWARNING %s 
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fsyntax-only  -verify 
-Wno-pointer-integer-compare %s 
+
+#ifdef EXPECTWARNING
+// expected-warning@+6 {{comparison between pointer and integer ('int' and 
'int *')}}
+#else
+// expected-no-diagnostics 
+#endif
+
+int test_ext_typecheck_comparison_of_pointer_integer(int integer, int * 
pointer) {
+   return integer != pointer; 
+}


Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5875,7 +5875,8 @@
 def err_typecheck_comparison_of_fptr_to_void : Error<
   "equality comparison between function pointer and void pointer (%0 and %1)">;
 def ext_typecheck_comparison_of_pointer_integer : ExtWarn<
-  "comparison between pointer and integer (%0 and %1)">;
+  "comparison between pointer and integer (%0 and %1)">,
+  InGroup>;
 def err_typecheck_comparison_of_pointer_integer : Error<
   "comparison between pointer and integer (%0 and %1)">;
 def ext_typecheck_comparison_of_distinct_pointers : ExtWarn<
Index: test/Misc/warning-flags.c
===
--- test/Misc/warning-flags.c
+++ test/Misc/warning-flags.c
@@ -18,7 +18,7 @@
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
-CHECK: Warnings without flags (75):
+CHECK: Warnings without flags (74):
 CHECK-NEXT:   ext_excess_initializers
 CHECK-NEXT:   ext_excess_initializers_in_char_array_initializer
 CHECK-NEXT:   ext_expected_semi_decl_list
@@ -29,7 +29,6 @@
 CHECK-NEXT:   ext_new_paren_array_nonconst
 CHECK-NEXT:   ext_plain_complex
 CHECK-NEXT:   ext_template_arg_extra_parens
-CHECK-NEXT:   ext_typecheck_comparison_of_pointer_integer
 CHECK-NEXT:   ext_typecheck_cond_incompatible_operands
 CHECK-NEXT:   ext_typecheck_ordered_comparison_of_pointer_integer
 CHECK-NEXT:   ext_using_undefined_std
Index: test/Sema/ext-typecheck-comparison-of-pointer-integer.c
===
--- test/Sema/ext-typecheck-comparison-of-pointer-integer.c
+++ test/Sema/ext-typecheck-comparison-of-pointer-integer.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fsyntax-only  -verify -DEXPECTWARNING %s 
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fsyntax-only  -verify -Wno-pointer-integer-compare %s 
+
+#ifdef EXPECTWARNING
+// expected-warning@+6 {{comparison between

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

[Kristina Brooks via Phabricator via cfe-commits]

kristina created this revision.
kristina added reviewers: rsmith, clang, JonasToth, EricWF, aaron.ballman.
Herald added subscribers: dexonsmith, mehdi_amini.
Herald added a reviewer: jfb.

Difficult to reproduce crash, attempted it in the test, it appears to not 
trigger it. I can consistently reproduce it when building a large project with 
this attribute added and can consistently crash it without the fix in place, 
however.

This avoids the codepath that should not be executed in the first place by 
rechecking `NoDestroyAttr` and bailing out instead of emitting incorrect code 
or causing an assert in assert enabled builds. This does not appear to cause 
any regressions for x86_64 test suite and my test is useless since it does not 
offer the needed coverage.

  clang-8: /SourceCache/llvm-trunk-8.0/include/llvm/Support/Casting.h:106: 
static bool llvm::isa_impl_cl::doit(const From *) [To = clang::CXXConstructorDecl, 
From = const clang::CXXMethodDecl *]: Assertion `Val && "isa<> used on a null 
pointer"' failed.
  
  -- asserts here, rest is signal handler/stack trace --
  
  #9 0x00d5a35d toCXXCtorType 
/SourceCache/llvm-trunk-8.0/tools/clang/lib/CodeGen/CodeGenTypes.h:66:3
  #10 0x00d5a35d 
clang::CodeGen::CodeGenModule::getAddrOfCXXStructor(clang::CXXMethodDecl 
const*, clang::CodeGen::StructorType, clang::CodeGen::CGFunctionInfo const*, 
llvm::FunctionType*, bool, clang::CodeGen::ForDefinition_t) 
/SourceCache/llvm-trunk-8.0/tools/clang/lib/CodeGen/CGCXX.cpp:237:0
  #11 0x00e05c9b Address 
/SourceCache/llvm-trunk-8.0/tools/clang/lib/CodeGen/Address.h:31:5
  #12 0x00e05c9b ConstantAddress 
/SourceCache/llvm-trunk-8.0/tools/clang/lib/CodeGen/Address.h:78:0
  #13 0x00e05c9b 
clang::CodeGen::CodeGenFunction::EmitCXXGlobalVarDeclInit(clang::VarDecl 
const&, llvm::Constant*, bool) 
/SourceCache/llvm-trunk-8.0/tools/clang/lib/CodeGen/CGDeclCXX.cpp:179:0
  #14 0x00e07b7a 
clang::CodeGen::CodeGenFunction::GenerateCXXGlobalVarDeclInitFunc(llvm::Function*,
 clang::VarDecl const*, llvm::GlobalVariable*, bool) 
/SourceCache/llvm-trunk-8.0/tools/clang/lib/CodeGen/CGDeclCXX.cpp:608:3
  #15 0x00e06fc8 
clang::CodeGen::CodeGenModule::EmitCXXGlobalVarDeclInitFunc(clang::VarDecl 
const*, llvm::GlobalVariable*, bool) 
/SourceCache/llvm-trunk-8.0/tools/clang/lib/CodeGen/CGDeclCXX.cpp:441:3
  #16 0x00b6596f 
clang::CodeGen::CodeGenModule::EmitGlobalVarDefinition(clang::VarDecl const*, 
bool) 
/SourceCache/llvm-trunk-8.0/tools/clang/lib/CodeGen/CodeGenModule.cpp:3650:5
  #17 0x00b5c66b 
clang::CodeGen::CodeGenModule::EmitGlobalDefinition(clang::GlobalDecl, 
llvm::GlobalValue*) 
/SourceCache/llvm-trunk-8.0/tools/clang/lib/CodeGen/CodeGenModule.cpp:0:12
  #18 0x00b67dfb getKind 
/SourceCache/llvm-trunk-8.0/tools/clang/include/clang/AST/DeclBase.h:421:51
  #19 0x00b67dfb classof 
/SourceCache/llvm-trunk-8.0/tools/clang/include/clang/AST/DeclCXX.h:3875:0
  #20 0x00b67dfb doit 
/SourceCache/llvm-trunk-8.0/include/llvm/Support/Casting.h:59:0
  #21 0x00b67dfb doit 
/SourceCache/llvm-trunk-8.0/include/llvm/Support/Casting.h:107:0
  #22 0x00b67dfb doit 
/SourceCache/llvm-trunk-8.0/include/llvm/Support/Casting.h:133:0
  #23 0x00b67dfb doit 
/SourceCache/llvm-trunk-8.0/include/llvm/Support/Casting.h:123:0
  #24 0x00b67dfb isa 
/SourceCache/llvm-trunk-8.0/include/llvm/Support/Casting.h:143:0
  #25 0x00b67dfb dyn_cast 
/SourceCache/llvm-trunk-8.0/include/llvm/Support/Casting.h:334:0
  #26 0x00b67dfb 
clang::CodeGen::CodeGenModule::EmitTopLevelDecl(clang::Decl*) 
/SourceCache/llvm-trunk-8.0/tools/clang/lib/CodeGen/CodeGenModule.cpp:4783:0
  #27 0x00b6bf7b getPointer 
/SourceCache/llvm-trunk-8.0/include/llvm/ADT/PointerIntPair.h:56:58
  #28 0x00b6bf7b getNextDeclInContext 
/SourceCache/llvm-trunk-8.0/tools/clang/include/clang/AST/DeclBase.h:424:0
  #29 0x00b6bf7b operator++ 
/SourceCache/llvm-trunk-8.0/tools/clang/include/clang/AST/DeclBase.h:1973:0
  #30 0x00b6bf7b 
clang::CodeGen::CodeGenModule::EmitDeclContext(clang::DeclContext const*) 
/SourceCache/llvm-trunk-8.0/tools/clang/lib/CodeGen/CodeGenModule.cpp:4744:0
  #31 0x0115a470 
_ZN12_GLOBAL__N_117CodeGeneratorImpl18HandleTopLevelDeclEN5clang12DeclGroupRefE$7cf5ba2c3e38da85712ae9dd9c2a6e32
 /SourceCache/llvm-trunk-8.0/tools/clang/lib/CodeGen/ModuleBuilder.cpp:159:73
  #32 0x0115833d 
clang::BackendConsumer::HandleTopLevelDecl(clang::DeclGroupRef) 
/SourceCache/llvm-trunk-8.0/tools/clang/lib/CodeGen/CodeGenAction.cpp:171:11
  #33 0x011642f4 clang::ParseAST(clang::Sema&, bool, bool) 
/SourceCache/llvm-trunk-8.0/tools/clang/lib/Parse/ParseAST.cpp:161:11
  #34 0x010c9d83 shouldBuildGlobalModuleIndex 
/SourceCache/llvm-trunk-8.0/tools/clang/lib/Frontend/CompilerInstance.cpp:81:11
  #35 0x010c9d83 clang::FrontendAction::Execute() 
/SourceCache/llvm-trunk-8.0/tools/clang/lib/Fr

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

[Kristina Brooks via Phabricator via cfe-commits]

kristina updated this revision to Diff 173411.
kristina added a comment.

Revised (style/ordering).


https://reviews.llvm.org/D54344

Files:
  lib/CodeGen/CGDeclCXX.cpp


Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -64,10 +64,19 @@
 /// static storage duration.
 static void EmitDeclDestroy(CodeGenFunction &CGF, const VarDecl &D,
 ConstantAddress addr) {
-  CodeGenModule &CGM = CGF.CGM;
+  // Workaround for a bug that causes a reference to a nonexistent
+  // destructor under odd circumstances, when attribute no_destroy
+  // is used. This code should not be reachable under normal 
+  // circumstances, this workaround simply checks for the attribute
+  // again and bails if it's present instead of following a path
+  // that's either going to assert or emit incorrect code if reached.
+  if (D.hasAttr())
+return;
 
   // FIXME:  __attribute__((cleanup)) ?
 
+  CodeGenModule &CGM = CGF.CGM;
+
   QualType type = D.getType();
   QualType::DestructionKind dtorKind = type.isDestructedType();
 


Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -64,10 +64,19 @@
 /// static storage duration.
 static void EmitDeclDestroy(CodeGenFunction &CGF, const VarDecl &D,
 ConstantAddress addr) {
-  CodeGenModule &CGM = CGF.CGM;
+  // Workaround for a bug that causes a reference to a nonexistent
+  // destructor under odd circumstances, when attribute no_destroy
+  // is used. This code should not be reachable under normal 
+  // circumstances, this workaround simply checks for the attribute
+  // again and bails if it's present instead of following a path
+  // that's either going to assert or emit incorrect code if reached.
+  if (D.hasAttr())
+return;
 
   // FIXME:  __attribute__((cleanup)) ?
 
+  CodeGenModule &CGM = CGF.CGM;
+
   QualType type = D.getType();
   QualType::DestructionKind dtorKind = type.isDestructedType();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

In https://reviews.llvm.org/D54344#1293468, @erik.pilkington wrote:

> Have you tried running creduce on the preprocessed source? We should really 
> have a real reproducer for this, otherwise its really hard to tell what the 
> underlying problem is.


Unable to build it with current Clang/LLVM headers, attribute is a very recent 
addition as well as general changes. I just get a flurry of errors like this:

  /src/clwn/creduce/clang_delta/InstantiateTemplateTypeParamToInt.cpp:187:19: 
error: no member named 'getLocStart' in 'clang::TemplateTypeParmTypeLoc'
void *Ptr = Loc.getLocStart().getPtrEncoding();


https://reviews.llvm.org/D54344



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

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

In https://reviews.llvm.org/D54344#1293643, @aaron.ballman wrote:

> In https://reviews.llvm.org/D54344#1293468, @erik.pilkington wrote:
>
> > Have you tried running creduce on the preprocessed source? We should really 
> > have a real reproducer for this, otherwise its really hard to tell what the 
> > underlying problem is.
>
>
> I'm going to echo this request -- without a test case, it's hard to know 
> whether this actually fixes anything or just moves the issue elsewhere.


Running the creduce on it now, it's slowly chugging along, will see what 
happens, only `15112502 bytes` to go, I'll get back to this in a few days and 
see what/if anything it comes up with. I do hope I launched it right.


https://reviews.llvm.org/D54344



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

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

This seems to be a peculiar side effect of weird combinations of previous uses 
of attributes `no_destroy`, `used`, and `section`, as well as complex macros 
used to create metaclass-like systems (through the use of said attributes). It 
seems that there is a serious bug somewhere, the repeated use of metaclass-like 
macros seem to break Clang's lexer/parser and leave it in a state where it 
starts behaving erratically, with this being a major symptom of it.

The trace state at the time of the crash usually resembles this:

  1.  /SourceCache/ips-1.0.0/os/credentials.cpp:20:41: current parser token 
';'
  2.  LLVM IR generation of declaration ''
  3.  /SourceCache/ips-1.0.0/os/credentials.cpp:20:1 
: Generating code for 
declaration '(anonymous namespace)::s_log'

When using `static` instead of an anonymous namespace the last token parsed is 
`static` instead of `;`. I also seem to have found a bug in implementation of 
`no_destroy`, fixing which would be considered an actual "fix" instead of a 
"workaround". I gave several other approaches of replicating this state, 
closest one being:

  class global_ns_class {
  public:
global_ns_class(int some_value) { SomeValue[2] = some_value; }
~global_ns_class() { SomeValue[2] = 1; }
  protected:
volatile int SomeValue[10];
  };
  
  #define MAKE_GVAR(_sfx, _init_val) \
  global_ns_class g_var##_sfx(_init_val); \
  __attribute((no_destroy, used, section("namedsect"))) \
  global_ns_class* g_var##_sfx##_ref = &g_var##_sfx;
  
  MAKE_GVAR(ONE,   1);
  MAKE_GVAR(TWO,   2);
  MAKE_GVAR(THREE, 3);
  
  namespace my_ns {
class with_nontrivial {
public:
with_nontrivial() { SomeValue[2] = 3; }
~with_nontrivial() { SomeValue[2] = 4; }
protected:
int SomeValue[10];
};

class my_class : public with_nontrivial {
public:
my_class(const char* cstr) {}
};
  }
  
  #define MAKE_VAR(_x,_y) \
namespace { constexpr char _$__##_x[] = _y; \
__attribute((no_destroy)) my_ns::my_class _x (_$__##_x); }

  MAKE_VAR(s_var, "test str lit");

Which seems to force Clang to take an incorrect code path but not trip the 
assert, unlike in the project where the assert is reliably tripped likely due 
to a manifestation of another, far more bening bug which is far beyond the 
scope of my understanding of Clang internals (it seems to be rooted in the 
preprocessor<->sema interactions). The 400k line preprocessed file is able to 
trigger it with 100% reliability, the preconditions for triggering the bug 
require the code to be semantically valid, which makes it hard to fuzz.

There seem to be too many variables to take into account to reliably reproduce 
it with the assert firing off, some manifestations just generate 
subtly-incorrect code (by emitting `__cxa_atexit` even with attribute present). 
The libc `atexit` fallback in handling of that attribute also seems like it 
could lead to issue but at this point this is just speculation.

I'm honestly not sure what to make of it at this point. Considering it's an 
immensely useful attribute for eliminating destructors of objects with 
"infinite" lifetime (I first hit this bug when I added it to all top level 
loggers which are created through a macro similar to one above), given that the 
x86_64 test suite does pass with it, I still think some kind of workaround is 
worth it (with reference to this issue)? I'd be happy to revert it if/once a 
concrete cause is found but so far I seem to be hitting dead ends.

I added a logger for when this specific issue is triggered to my downstream 
fork and it seems in most cases it causes "bening" behavior of attribute being 
ineffective. The assert is tripped when the attribute is partially ineffective, 
leading to Clang trying to emit code to register a destructor that doesn't 
exist with cxxabi.

Considering this is something I will likely have to dig through by hand, can 
anyone help me brainstorm how to approach this besides fuzzing it which I don't 
think will work due to the precondition of the code being semantically valid 
(not being diagnosed with an error)?


https://reviews.llvm.org/D54344



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

[PATCH] D54373: [clang]: Correct naming conventions and 80 col rule violation in CGDeclCXX.cpp. NFC.

[Kristina Brooks via Phabricator via cfe-commits]

kristina created this revision.
kristina added a reviewer: clang.
Herald added a subscriber: cfe-commits.

Noticed while working with that area of code, NFC.


Repository:
  rC Clang

https://reviews.llvm.org/D54373

Files:
  lib/CodeGen/CGDeclCXX.cpp


Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -68,10 +68,10 @@
 
   // FIXME:  __attribute__((cleanup)) ?
 
-  QualType type = D.getType();
-  QualType::DestructionKind dtorKind = type.isDestructedType();
+  QualType Type = D.getType();
+  QualType::DestructionKind DtorKind = Type.isDestructedType();
 
-  switch (dtorKind) {
+  switch (DtorKind) {
   case QualType::DK_none:
 return;
 
@@ -86,13 +86,14 @@
 return;
   }
 
-  llvm::Constant *function;
-  llvm::Constant *argument;
+  llvm::Constant *Func;
+  llvm::Constant *Argument;
 
   // Special-case non-array C++ destructors, if they have the right signature.
   // Under some ABIs, destructors return this instead of void, and cannot be
-  // passed directly to __cxa_atexit if the target does not allow this 
mismatch.
-  const CXXRecordDecl *Record = type->getAsCXXRecordDecl();
+  // passed directly to __cxa_atexit if the target does not allow this
+  // mismatch.
+  const CXXRecordDecl *Record = Type->getAsCXXRecordDecl();
   bool CanRegisterDestructor =
   Record && (!CGM.getCXXABI().HasThisReturn(
  GlobalDecl(Record->getDestructor(), Dtor_Complete)) ||
@@ -103,21 +104,21 @@
   bool UsingExternalHelper = !CGM.getCodeGenOpts().CXAAtExit;
   if (Record && (CanRegisterDestructor || UsingExternalHelper)) {
 assert(!Record->hasTrivialDestructor());
-CXXDestructorDecl *dtor = Record->getDestructor();
+CXXDestructorDecl *Dtor = Record->getDestructor();
 
-function = CGM.getAddrOfCXXStructor(dtor, StructorType::Complete);
-argument = llvm::ConstantExpr::getBitCast(
-addr.getPointer(), CGF.getTypes().ConvertType(type)->getPointerTo());
+Func = CGM.getAddrOfCXXStructor(Dtor, StructorType::Complete);
+Argument = llvm::ConstantExpr::getBitCast(
+Addr.getPointer(), CGF.getTypes().ConvertType(Type)->getPointerTo());
 
   // Otherwise, the standard logic requires a helper function.
   } else {
-function = CodeGenFunction(CGM)
-.generateDestroyHelper(addr, type, CGF.getDestroyer(dtorKind),
-   CGF.needsEHCleanup(dtorKind), &D);
-argument = llvm::Constant::getNullValue(CGF.Int8PtrTy);
+Func = CodeGenFunction(CGM)
+   .generateDestroyHelper(Addr, Type, CGF.getDestroyer(DtorKind),
+  CGF.needsEHCleanup(DtorKind), &D);
+Argument = llvm::Constant::getNullValue(CGF.Int8PtrTy);
   }
 
-  CGM.getCXXABI().registerGlobalDtor(CGF, D, function, argument);
+  CGM.getCXXABI().registerGlobalDtor(CGF, D, Func, Argument);
 }
 
 /// Emit code to cause the variable at the given address to be considered as


Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -68,10 +68,10 @@
 
   // FIXME:  __attribute__((cleanup)) ?
 
-  QualType type = D.getType();
-  QualType::DestructionKind dtorKind = type.isDestructedType();
+  QualType Type = D.getType();
+  QualType::DestructionKind DtorKind = Type.isDestructedType();
 
-  switch (dtorKind) {
+  switch (DtorKind) {
   case QualType::DK_none:
 return;
 
@@ -86,13 +86,14 @@
 return;
   }
 
-  llvm::Constant *function;
-  llvm::Constant *argument;
+  llvm::Constant *Func;
+  llvm::Constant *Argument;
 
   // Special-case non-array C++ destructors, if they have the right signature.
   // Under some ABIs, destructors return this instead of void, and cannot be
-  // passed directly to __cxa_atexit if the target does not allow this mismatch.
-  const CXXRecordDecl *Record = type->getAsCXXRecordDecl();
+  // passed directly to __cxa_atexit if the target does not allow this
+  // mismatch.
+  const CXXRecordDecl *Record = Type->getAsCXXRecordDecl();
   bool CanRegisterDestructor =
   Record && (!CGM.getCXXABI().HasThisReturn(
  GlobalDecl(Record->getDestructor(), Dtor_Complete)) ||
@@ -103,21 +104,21 @@
   bool UsingExternalHelper = !CGM.getCodeGenOpts().CXAAtExit;
   if (Record && (CanRegisterDestructor || UsingExternalHelper)) {
 assert(!Record->hasTrivialDestructor());
-CXXDestructorDecl *dtor = Record->getDestructor();
+CXXDestructorDecl *Dtor = Record->getDestructor();
 
-function = CGM.getAddrOfCXXStructor(dtor, StructorType::Complete);
-argument = llvm::ConstantExpr::getBitCast(
-addr.getPointer(), CGF.getTypes().ConvertType(type)->getPointerTo());
+Func = CGM.getAddrOfCXXStructor(Dtor, StructorType::Complete);
+Argument = llvm::ConstantExpr::getBitCast(
+Addr.getPointer(), CGF.getTypes().ConvertType(Type)->getPointerTo());
 
   // Ot

[PATCH] D54373: [clang]: Correct naming conventions and 80 col rule violation in CGDeclCXX.cpp. NFC.

[Kristina Brooks via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346582: Correct naming conventions and 80 col rule violation 
in CGDeclCXX.cpp. NFC. (authored by kristina, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54373?vs=173493&id=173494#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D54373

Files:
  cfe/trunk/lib/CodeGen/CGDeclCXX.cpp


Index: cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
===
--- cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
+++ cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
@@ -68,10 +68,10 @@
 
   // FIXME:  __attribute__((cleanup)) ?
 
-  QualType type = D.getType();
-  QualType::DestructionKind dtorKind = type.isDestructedType();
+  QualType Type = D.getType();
+  QualType::DestructionKind DtorKind = Type.isDestructedType();
 
-  switch (dtorKind) {
+  switch (DtorKind) {
   case QualType::DK_none:
 return;
 
@@ -86,13 +86,14 @@
 return;
   }
 
-  llvm::Constant *function;
-  llvm::Constant *argument;
+  llvm::Constant *Func;
+  llvm::Constant *Argument;
 
   // Special-case non-array C++ destructors, if they have the right signature.
   // Under some ABIs, destructors return this instead of void, and cannot be
-  // passed directly to __cxa_atexit if the target does not allow this 
mismatch.
-  const CXXRecordDecl *Record = type->getAsCXXRecordDecl();
+  // passed directly to __cxa_atexit if the target does not allow this
+  // mismatch.
+  const CXXRecordDecl *Record = Type->getAsCXXRecordDecl();
   bool CanRegisterDestructor =
   Record && (!CGM.getCXXABI().HasThisReturn(
  GlobalDecl(Record->getDestructor(), Dtor_Complete)) ||
@@ -103,21 +104,21 @@
   bool UsingExternalHelper = !CGM.getCodeGenOpts().CXAAtExit;
   if (Record && (CanRegisterDestructor || UsingExternalHelper)) {
 assert(!Record->hasTrivialDestructor());
-CXXDestructorDecl *dtor = Record->getDestructor();
+CXXDestructorDecl *Dtor = Record->getDestructor();
 
-function = CGM.getAddrOfCXXStructor(dtor, StructorType::Complete);
-argument = llvm::ConstantExpr::getBitCast(
-addr.getPointer(), CGF.getTypes().ConvertType(type)->getPointerTo());
+Func = CGM.getAddrOfCXXStructor(Dtor, StructorType::Complete);
+Argument = llvm::ConstantExpr::getBitCast(
+Addr.getPointer(), CGF.getTypes().ConvertType(Type)->getPointerTo());
 
   // Otherwise, the standard logic requires a helper function.
   } else {
-function = CodeGenFunction(CGM)
-.generateDestroyHelper(addr, type, CGF.getDestroyer(dtorKind),
-   CGF.needsEHCleanup(dtorKind), &D);
-argument = llvm::Constant::getNullValue(CGF.Int8PtrTy);
+Func = CodeGenFunction(CGM)
+   .generateDestroyHelper(Addr, Type, CGF.getDestroyer(DtorKind),
+  CGF.needsEHCleanup(DtorKind), &D);
+Argument = llvm::Constant::getNullValue(CGF.Int8PtrTy);
   }
 
-  CGM.getCXXABI().registerGlobalDtor(CGF, D, function, argument);
+  CGM.getCXXABI().registerGlobalDtor(CGF, D, Func, Argument);
 }
 
 /// Emit code to cause the variable at the given address to be considered as


Index: cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
===
--- cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
+++ cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
@@ -68,10 +68,10 @@
 
   // FIXME:  __attribute__((cleanup)) ?
 
-  QualType type = D.getType();
-  QualType::DestructionKind dtorKind = type.isDestructedType();
+  QualType Type = D.getType();
+  QualType::DestructionKind DtorKind = Type.isDestructedType();
 
-  switch (dtorKind) {
+  switch (DtorKind) {
   case QualType::DK_none:
 return;
 
@@ -86,13 +86,14 @@
 return;
   }
 
-  llvm::Constant *function;
-  llvm::Constant *argument;
+  llvm::Constant *Func;
+  llvm::Constant *Argument;
 
   // Special-case non-array C++ destructors, if they have the right signature.
   // Under some ABIs, destructors return this instead of void, and cannot be
-  // passed directly to __cxa_atexit if the target does not allow this mismatch.
-  const CXXRecordDecl *Record = type->getAsCXXRecordDecl();
+  // passed directly to __cxa_atexit if the target does not allow this
+  // mismatch.
+  const CXXRecordDecl *Record = Type->getAsCXXRecordDecl();
   bool CanRegisterDestructor =
   Record && (!CGM.getCXXABI().HasThisReturn(
  GlobalDecl(Record->getDestructor(), Dtor_Complete)) ||
@@ -103,21 +104,21 @@
   bool UsingExternalHelper = !CGM.getCodeGenOpts().CXAAtExit;
   if (Record && (CanRegisterDestructor || UsingExternalHelper)) {
 assert(!Record->hasTrivialDestructor());
-CXXDestructorDecl *dtor = Record->getDestructor();
+CXXDestructorDecl *Dtor = Record->getDestructor();
 
-function = CGM.getAddrOfCXXStructor(dtor, StructorType::Complete);
-argument = llvm::ConstantExpr::getBitCast(
-addr.getPointer(), CGF.

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

I've managed to isolate enough to make for a testcase. Is that too broad or is 
it okay to attach as is?

The following triggers the assert:

  namespace util {
template 
class test_vector
{
public:
test_vector(unsigned c)
: Used(0), TotalSize(sizeof(T) * c)
{
// Intentional
Storage = (T*)(new T[TotalSize]);
}
~test_vector() {
delete[] Storage;
}
char* data() {
return Storage;
}
unsigned size() {
return TotalSize;
}
void push_back(T one_thing) {
Storage[Used++] = one_thing;
}
private:
unsigned Used;
unsigned TotalSize;
T*   Storage;
};
  }
  
  volatile int do_not_optimize_me = 0;
  
  namespace os {
using char_vec_t = util::test_vector;

class logger_base
{
public:
constexpr logger_base() = default;
protected:
char_vec_t* NamePtr = nullptr;
char_vec_t  Name= char_vec_t(10);
};

class logger : public logger_base
{
public:
void stuff(const char* fmt, int something) {
do_not_optimize_me = something + fmt[0];
}
logger()
{
Name = char_vec_t(8);
Name.push_back('a');
}

logger(const char* name)
{
Name = util::test_vector(__builtin_strlen(name));
Name.push_back(name[0]);
Name.push_back(name[1]);
}
};
  }
  
  #define TOPLEVEL_LOGGER(_var_name, _category_const)   \
namespace { constexpr char $__##_var_name[] = _category_const; \
__attribute((no_destroy)) os::logger _var_name ($__##_var_name) ; }
  
  TOPLEVEL_LOGGER(s_log, "something");
  
  class some_class {
  public:
some_class(int some_value) {
do_not_optimize_me = some_value;
s_log.stuff("wawawa", 5 + do_not_optimize_me);
}
~some_class() = default;
  };
  
  static some_class s_ctor(1);


https://reviews.llvm.org/D54344



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

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

[Kristina Brooks via Phabricator via cfe-commits]

kristina updated this revision to Diff 173503.
kristina set the repository for this revision to rC Clang.
kristina added a comment.

Revised, I think I worked out what was happening, there's an explanation in the 
comment in the test. This is a relatively new attribute so the coverage for it 
did not test this specific corner case, I've managed to manually narrow it down 
and write a reliable regression test. The core issue boils down to having a 
non-trivially destructible member in a superclass that lacks a destructor and a 
subclass that lacks a destructor, in which case, a different path was taken to 
balance out the static storage duration class' initializer call and the 
`__cxa_atexit` registration. This adds an explicit check for the attribute and 
avoids balancing out the constructor as intended by the attribute.

The new test successfully replicates the assertion failure and should fail for 
the above cases in assertion builds. Without assertions the generated code 
produces undefined behavior if the above conditions are met. There is a 
separate test for this attribute that provides the coverage for its 
functionality, this is a much more narrower test for a very specific scenario 
in which it was possible to cause an improperly balanced constructor call 
followed by a emission of a call to a destructor that would never be emitted 
due to the attribute, thus tripping the assert. Now no attempt to call the 
destructor is made if the declaration is marked as `no_destroy`.

`Test without patch:`

  => ninja check-clang-semacxx
  
  FAIL: Clang :: SemaCXX/attr-no-destroy-d54344.cpp (164 of 818)
   TEST 'Clang :: SemaCXX/attr-no-destroy-d54344.cpp' 
FAILED 
  
  (--- stack trace and the expected assertion failure ---)
  
  
/q/org.llvm.caches/llvm-8.0/4141/tools/clang/test/SemaCXX/Output/attr-no-destroy-d54344.cpp.script:
 line 1: 31356 Aborted (core dumped)
  
  --
  
  
  Testing Time: 5.03s
  
  Failing Tests (1):
  Clang :: SemaCXX/attr-no-destroy-d54344.cpp
  
Expected Passes: 816
Expected Failures  : 1
Unexpected Failures: 1
  FAILED: tools/clang/test/CMakeFiles/check-clang-semacxx
  
  ninja: build stopped: subcommand failed.

`Patch applied:`

  => ninja check-clang-semacxx
  
  Testing Time: 6.28s
Expected Passes: 817
Expected Failures  : 1


Repository:
  rC Clang

https://reviews.llvm.org/D54344

Files:
  lib/CodeGen/CGDeclCXX.cpp
  test/SemaCXX/attr-no-destroy-d54344.cpp

Index: test/SemaCXX/attr-no-destroy-d54344.cpp
===
--- test/SemaCXX/attr-no-destroy-d54344.cpp
+++ test/SemaCXX/attr-no-destroy-d54344.cpp
@@ -0,0 +1,92 @@
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -S -O0 %s -o - > /dev/null
+// REQUIRES: asserts
+
+// Regression test for D54344. Class with no user-defined destructor
+// that has an inherited member that has a non-trivial destructor
+// and a non-default constructor will attempt to emit a destructor
+// despite being marked as __attribute((no_destroy)) in which case
+// it would trigger an assertion due to an incorrect assumption.
+// This test requires asserts to reliably work as without the crash
+// it generates working but semantically incorrect code.
+
+namespace util {
+	template 
+	class test_vector
+	{
+	public:
+		test_vector(unsigned c)
+			: Used(0), TotalSize(sizeof(T) * c)
+		{
+			// Intentional
+			Storage = (T*)(new T[TotalSize]);
+		}
+		~test_vector() {
+			delete[] Storage;
+		}
+		char* data() {
+			return Storage;
+		}
+		unsigned size() {
+			return TotalSize;
+		}
+		void push_back(T one_thing) {
+			Storage[Used++] = one_thing;
+		}
+	private:
+		unsigned Used;
+		unsigned TotalSize;
+		T*   Storage;
+	};
+}
+
+volatile int do_not_optimize_me = 0;
+
+namespace os {
+	using char_vec_t = util::test_vector;
+	
+	class logger_base
+	{
+	public:
+		constexpr logger_base() = default;
+	protected:
+		char_vec_t* NamePtr = nullptr;
+		char_vec_t  Name= char_vec_t(10);
+	};
+	
+	class logger : public logger_base
+	{
+	public:
+		void stuff(const char* fmt, int something) {
+			do_not_optimize_me = something + fmt[0];
+		}
+		logger()
+		{
+			Name = char_vec_t(8);
+			Name.push_back('a');
+		}
+		
+		logger(const char* name)
+		{
+			Name = util::test_vector(__builtin_strlen(name));
+			Name.push_back(name[0]);
+			Name.push_back(name[1]);
+		}
+	};
+}
+
+#define TOPLEVEL_LOGGER(_var_name, _category_const)   \
+	namespace { constexpr char $__##_var_name[] = _category_const; \
+	__attribute((no_destroy)) os::logger _var_name ($__##_var_name) ; }
+
+TOPLEVEL_LOGGER(s_log, "something");
+
+class some_class {
+public:
+	some_class(int some_value) {
+		do_not_optimize_me = some_value;
+		s_log.stuff("wawawa", 5 + do_not_optimize_me);
+	}
+	~some_class() = default;
+};
+
+static some_class s_ctor(1);
\ No newline at end of file
Index: lib/CodeGen/CGDeclCXX.cpp
==

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

[Kristina Brooks via Phabricator via cfe-commits]

kristina updated this revision to Diff 173507.
kristina added a comment.

Revised, added a newline to the testcase.


https://reviews.llvm.org/D54344

Files:
  lib/CodeGen/CGDeclCXX.cpp
  test/SemaCXX/attr-no-destroy-d54344.cpp

Index: test/SemaCXX/attr-no-destroy-d54344.cpp
===
--- test/SemaCXX/attr-no-destroy-d54344.cpp
+++ test/SemaCXX/attr-no-destroy-d54344.cpp
@@ -0,0 +1,92 @@
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -S -O0 %s -o - > /dev/null
+// REQUIRES: asserts
+
+// Regression test for D54344. Class with no user-defined destructor
+// that has an inherited member that has a non-trivial destructor
+// and a non-default constructor will attempt to emit a destructor
+// despite being marked as __attribute((no_destroy)) in which case
+// it would trigger an assertion due to an incorrect assumption.
+// This test requires asserts to reliably work as without the crash
+// it generates working but semantically incorrect code.
+
+namespace util {
+	template 
+	class test_vector
+	{
+	public:
+		test_vector(unsigned c)
+			: Used(0), TotalSize(sizeof(T) * c)
+		{
+			// Intentional
+			Storage = (T*)(new T[TotalSize]);
+		}
+		~test_vector() {
+			delete[] Storage;
+		}
+		char* data() {
+			return Storage;
+		}
+		unsigned size() {
+			return TotalSize;
+		}
+		void push_back(T one_thing) {
+			Storage[Used++] = one_thing;
+		}
+	private:
+		unsigned Used;
+		unsigned TotalSize;
+		T*   Storage;
+	};
+}
+
+volatile int do_not_optimize_me = 0;
+
+namespace os {
+	using char_vec_t = util::test_vector;
+	
+	class logger_base
+	{
+	public:
+		constexpr logger_base() = default;
+	protected:
+		char_vec_t* NamePtr = nullptr;
+		char_vec_t  Name= char_vec_t(10);
+	};
+	
+	class logger : public logger_base
+	{
+	public:
+		void stuff(const char* fmt, int something) {
+			do_not_optimize_me = something + fmt[0];
+		}
+		logger()
+		{
+			Name = char_vec_t(8);
+			Name.push_back('a');
+		}
+		
+		logger(const char* name)
+		{
+			Name = util::test_vector(__builtin_strlen(name));
+			Name.push_back(name[0]);
+			Name.push_back(name[1]);
+		}
+	};
+}
+
+#define TOPLEVEL_LOGGER(_var_name, _category_const)   \
+	namespace { constexpr char $__##_var_name[] = _category_const; \
+	__attribute((no_destroy)) os::logger _var_name ($__##_var_name) ; }
+
+TOPLEVEL_LOGGER(s_log, "something");
+
+class some_class {
+public:
+	some_class(int some_value) {
+		do_not_optimize_me = some_value;
+		s_log.stuff("wawawa", 5 + do_not_optimize_me);
+	}
+	~some_class() = default;
+};
+
+static some_class s_ctor(1);
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -64,6 +64,14 @@
 /// static storage duration.
 static void EmitDeclDestroy(CodeGenFunction &CGF, const VarDecl &D,
 ConstantAddress Addr) {
+  // Honor __attribute__((no_destroy)) and bail instead of attempting
+  // to emit a reference to a possibly non-existant destructor, which
+  // in turn can cause a crash. This will result in a global constructor
+  // that isn't balanced out by a destructor call as intended by the
+  // attribute (D54344).
+  if (D.hasAttr())
+return;
+  
   CodeGenModule &CGM = CGF.CGM;
 
   // FIXME:  __attribute__((cleanup)) ?
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54379: Add Hurd support

[Kristina Brooks via Phabricator via cfe-commits]

kristina added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:533
 
-  if (Triple.isOSLinux() || Triple.getOS() == llvm::Triple::CloudABI) {
+  if (Triple.isOSLinux() || Triple.getOS() == llvm::Triple::CloudABI || 
Triple.isOSHurd()) {
 switch (Triple.getArch()) {

Please keep line lengths to 80 columns at most, it's one of the few hard rules 
outlined in the developer policy.


Repository:
  rC Clang

https://reviews.llvm.org/D54379



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

[PATCH] D54379: Add Hurd support

[Kristina Brooks via Phabricator via cfe-commits]

kristina added reviewers: kristina, clang.
kristina added a comment.

A few style naming/comments.




Comment at: lib/Driver/ToolChains/Hurd.cpp:36
+  // clever.
+  switch (TargetTriple.getArch()) {
+  default:

Does this need a switch? Wouldn't an `if` be sufficient?



Comment at: lib/Driver/ToolChains/Hurd.cpp:106
+CIncludeDirs.split(dirs, ":");
+for (StringRef dir : dirs) {
+  StringRef Prefix =

Variable names should be capitalized.



Comment at: lib/Driver/ToolChains/Hurd.cpp:125
+break;
+  default:
+break;

Default should generally be the first case, if present.


Repository:
  rC Clang

https://reviews.llvm.org/D54379



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

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

[Kristina Brooks via Phabricator via cfe-commits]

kristina updated this revision to Diff 173530.
kristina set the repository for this revision to rC Clang.
kristina added a comment.

@erik.pilkington Fantastic catch, I totally forgot about the global flag. 
Revised, moved to CodeGenCXX, added a test to verify ctor/dtor is balanced 
under normal circumstances and two tests to ensure ctor is not balanced with 
either flag or attribute.

  => ninja check-clang-codegencxx
  
  Testing Time: 7.68s
Expected Passes: 927
Expected Failures  : 2
Unsupported Tests  : 5

The attribute was actually missing tests for IR generation so thank you for 
suggesting moving them there, I think this covers all cases for this bug 
(balanced regression test without attr or flag, unbalanced test because of 
attr, unbalanced test because of global flag).


Repository:
  rC Clang

https://reviews.llvm.org/D54344

Files:
  lib/CodeGen/CGDeclCXX.cpp
  test/CodeGenCXX/attr-no-destroy-d54344.cpp


Index: test/CodeGenCXX/attr-no-destroy-d54344.cpp
===
--- test/CodeGenCXX/attr-no-destroy-d54344.cpp
+++ test/CodeGenCXX/attr-no-destroy-d54344.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu 
-DNOATTR  %s -o - | FileCheck %s
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu 
%s -o - | FileCheck %s --check-prefix=CHECK-ATTR
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu 
-DNOATTR -fno-c++-static-destructors %s -o - | FileCheck %s 
--check-prefix=CHECK-FLAG 
+// REQUIRES: asserts
+
+// Regression test for D54344. Class with no user-defined destructor
+// that has an inherited member that has a non-trivial destructor
+// and a non-default constructor will attempt to emit a destructor
+// despite being marked as __attribute((no_destroy)) in which case
+// it would trigger an assertion due to an incorrect assumption.
+// This test requires asserts to reliably work as without the crash
+// it generates working but semantically incorrect code.
+
+class a {
+public:
+  ~a();
+};
+class logger_base {
+  a d;
+};
+class e : logger_base {};
+#ifndef NOATTR
+__attribute((no_destroy))
+#endif
+e g;
+
+// In the absence of the attribute and flag, both ctor and dtor should
+// be emitted, check for that.
+// CHECK: @__cxx_global_var_init
+// CHECK: @__cxa_atexit
+
+// When attribute is enabled, the constructor should not be balanced
+// by a destructor. Make sure we have the ctor but not the dtor
+// registration.
+// CHECK-ATTR: @__cxx_global_var_init
+// CHECK-ATTR-NOT: @__cxa_atexit
+
+// Same scenario except with global flag (-fno-c++-static-destructors)
+// supressing it instead of the attribute. 
+// CHECK-FLAG: @__cxx_global_var_init
+// CHECK-FLAG-NOT: @__cxa_atexit
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -64,6 +64,15 @@
 /// static storage duration.
 static void EmitDeclDestroy(CodeGenFunction &CGF, const VarDecl &D,
 ConstantAddress Addr) {
+  // Honor __attribute__((no_destroy)) and bail instead of attempting
+  // to emit a reference to a possibly non-existant destructor, which
+  // in turn can cause a crash. This will result in a global constructor
+  // that isn't balanced out by a destructor call as intended by the
+  // attribute. This also checks for -fno-c++-static-destructors and
+  // bails even if the attribute is not present. (D54344)
+  if (D.isNoDestroy(CGF.getContext()))
+return;
+  
   CodeGenModule &CGM = CGF.CGM;
 
   // FIXME:  __attribute__((cleanup)) ?


Index: test/CodeGenCXX/attr-no-destroy-d54344.cpp
===
--- test/CodeGenCXX/attr-no-destroy-d54344.cpp
+++ test/CodeGenCXX/attr-no-destroy-d54344.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu -DNOATTR  %s -o - | FileCheck %s
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu %s -o - | FileCheck %s --check-prefix=CHECK-ATTR
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu -DNOATTR -fno-c++-static-destructors %s -o - | FileCheck %s --check-prefix=CHECK-FLAG 
+// REQUIRES: asserts
+
+// Regression test for D54344. Class with no user-defined destructor
+// that has an inherited member that has a non-trivial destructor
+// and a non-default constructor will attempt to emit a destructor
+// despite being marked as __attribute((no_destroy)) in which case
+// it would trigger an assertion due to an incorrect assumption.
+// This test requires asserts to reliably work as without the crash
+// it generates working but semantically incorrect code.
+
+class a {
+public:
+  ~a();
+};
+class logger_base {
+  a d;
+};
+class e : logger_base {};
+#ifndef NOATTR
+__attribute((no_destroy))
+#endif
+e g;
+
+// In the absence of the

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

In https://reviews.llvm.org/D53263#1294336, @bobsayshilol wrote:

> Thanks!
>  I don't have commit access to land this myself.


I can do it if you'd like, will be a moment though.


https://reviews.llvm.org/D53263



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

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

[Kristina Brooks via Phabricator via cfe-commits]

kristina updated this revision to Diff 173534.
kristina added a comment.

Revised to address nitpicks.


Repository:
  rC Clang

https://reviews.llvm.org/D54344

Files:
  lib/CodeGen/CGDeclCXX.cpp
  test/CodeGenCXX/attr-no-destroy-d54344.cpp


Index: test/CodeGenCXX/attr-no-destroy-d54344.cpp
===
--- test/CodeGenCXX/attr-no-destroy-d54344.cpp
+++ test/CodeGenCXX/attr-no-destroy-d54344.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu 
-DNOATTR  %s -o - | FileCheck %s
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu 
%s -o - | FileCheck %s --check-prefix=CHECK-ATTR
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu 
-DNOATTR -fno-c++-static-destructors %s -o - | FileCheck %s 
--check-prefix=CHECK-FLAG 
+// REQUIRES: asserts
+
+// Regression test for D54344. Class with no user-defined destructor
+// that has an inherited member that has a non-trivial destructor
+// and a non-default constructor will attempt to emit a destructor
+// despite being marked as __attribute((no_destroy)) in which case
+// it would trigger an assertion due to an incorrect assumption.
+// This test requires asserts to reliably work as without the crash
+// it generates working but semantically incorrect code.
+
+class a {
+public:
+  ~a();
+};
+class logger_base {
+  a d;
+};
+class e : logger_base {};
+#ifndef NOATTR
+__attribute((no_destroy))
+#endif
+e g;
+
+// In the absence of the attribute and flag, both ctor and dtor should
+// be emitted, check for that.
+// CHECK: @__cxx_global_var_init
+// CHECK: @__cxa_atexit
+
+// When attribute is enabled, the constructor should not be balanced
+// by a destructor. Make sure we have the ctor but not the dtor
+// registration.
+// CHECK-ATTR: @__cxx_global_var_init
+// CHECK-ATTR-NOT: @__cxa_atexit
+
+// Same scenario except with global flag (-fno-c++-static-destructors)
+// supressing it instead of the attribute. 
+// CHECK-FLAG: @__cxx_global_var_init
+// CHECK-FLAG-NOT: @__cxa_atexit
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -64,6 +64,15 @@
 /// static storage duration.
 static void EmitDeclDestroy(CodeGenFunction &CGF, const VarDecl &D,
 ConstantAddress Addr) {
+  // Honor __attribute__((no_destroy)) and bail instead of attempting
+  // to emit a reference to a possibly nonexistent destructor, which
+  // in turn can cause a crash. This will result in a global constructor
+  // that isn't balanced out by a destructor call as intended by the
+  // attribute. This also checks for -fno-c++-static-destructors and
+  // bails even if the attribute is not present.
+  if (D.isNoDestroy(CGF.getContext()))
+return;
+  
   CodeGenModule &CGM = CGF.CGM;
 
   // FIXME:  __attribute__((cleanup)) ?


Index: test/CodeGenCXX/attr-no-destroy-d54344.cpp
===
--- test/CodeGenCXX/attr-no-destroy-d54344.cpp
+++ test/CodeGenCXX/attr-no-destroy-d54344.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu -DNOATTR  %s -o - | FileCheck %s
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu %s -o - | FileCheck %s --check-prefix=CHECK-ATTR
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu -DNOATTR -fno-c++-static-destructors %s -o - | FileCheck %s --check-prefix=CHECK-FLAG 
+// REQUIRES: asserts
+
+// Regression test for D54344. Class with no user-defined destructor
+// that has an inherited member that has a non-trivial destructor
+// and a non-default constructor will attempt to emit a destructor
+// despite being marked as __attribute((no_destroy)) in which case
+// it would trigger an assertion due to an incorrect assumption.
+// This test requires asserts to reliably work as without the crash
+// it generates working but semantically incorrect code.
+
+class a {
+public:
+  ~a();
+};
+class logger_base {
+  a d;
+};
+class e : logger_base {};
+#ifndef NOATTR
+__attribute((no_destroy))
+#endif
+e g;
+
+// In the absence of the attribute and flag, both ctor and dtor should
+// be emitted, check for that.
+// CHECK: @__cxx_global_var_init
+// CHECK: @__cxa_atexit
+
+// When attribute is enabled, the constructor should not be balanced
+// by a destructor. Make sure we have the ctor but not the dtor
+// registration.
+// CHECK-ATTR: @__cxx_global_var_init
+// CHECK-ATTR-NOT: @__cxa_atexit
+
+// Same scenario except with global flag (-fno-c++-static-destructors)
+// supressing it instead of the attribute. 
+// CHECK-FLAG: @__cxx_global_var_init
+// CHECK-FLAG-NOT: @__cxa_atexit
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCX

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

In https://reviews.llvm.org/D53263#1294412, @bobsayshilol wrote:

> In https://reviews.llvm.org/D53263#1294383, @kristina wrote:
>
> > I can do it if you'd like, will be a moment though.
>
>
> Thanks and much appreciated!


Huge apologies, it seems I can't get this to patch cleanly against my fork and 
therefore can't test it before committing, which is something I generally 
always do. I'll leave it to someone else. Again, huge apologies, hopefully you 
won't have to wait too long.


https://reviews.llvm.org/D53263



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

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

In https://reviews.llvm.org/D53263#1294488, @JDevlieghere wrote:

> The patch applies for me but has quite a few style violations. I'll fix those 
> up before landing it.


Thank you and sorry for the trouble, my fork seems to have enough modifications 
to some of these files to confuse `patch` and getting an untainted checkout and 
integrating it for a single build/test run would be troublesome to undo.


Repository:
  rL LLVM

https://reviews.llvm.org/D53263



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

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

In https://reviews.llvm.org/D54344#1294942, @erik.pilkington wrote:

> LGTM too, with one small fix in test. Thanks for working on this!


Well, I figured since the assertion being tripped was (before investigation) 
the only reliable way to notice the bug it makes sense for it to stay in, main 
concern being that should anything regress and assertions are off, the code 
generation is essentially undefined. Though a good argument for taking it out 
would be the fact that currently it's the only test that verifies IR generated 
with the attribute (last time I checked), but I would also imagine most people 
running tests would have assertion enabled (or debug) builds.

Though I'm happy to revise the test to remove the assertion requirement, 
deferring to your judgement with regards to above.

Aside from that, are you happy for me to land this after the revision?


Repository:
  rC Clang

https://reviews.llvm.org/D54344



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

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

[Kristina Brooks via Phabricator via cfe-commits]

kristina updated this revision to Diff 173602.
kristina added a comment.

Revised to address comments.


Repository:
  rC Clang

https://reviews.llvm.org/D54344

Files:
  lib/CodeGen/CGDeclCXX.cpp
  test/CodeGenCXX/attr-no-destroy-d54344.cpp


Index: test/CodeGenCXX/attr-no-destroy-d54344.cpp
===
--- test/CodeGenCXX/attr-no-destroy-d54344.cpp
+++ test/CodeGenCXX/attr-no-destroy-d54344.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu 
-DNOATTR  %s -o - | FileCheck %s
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu 
%s -o - | FileCheck %s --check-prefix=CHECK-ATTR
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu 
-DNOATTR -fno-c++-static-destructors %s -o - | FileCheck %s 
--check-prefix=CHECK-FLAG
+
+// Regression test for D54344. Class with no user-defined destructor
+// that has an inherited member that has a non-trivial destructor
+// and a non-default constructor will attempt to emit a destructor
+// despite being marked as __attribute((no_destroy)) in which case
+// it would trigger an assertion due to an incorrect assumption.
+// This test requires asserts to reliably work as without the crash
+// it generates working but semantically incorrect code.
+
+class a {
+public:
+  ~a();
+};
+class logger_base {
+  a d;
+};
+class e : logger_base {};
+#ifndef NOATTR
+__attribute((no_destroy))
+#endif
+e g;
+
+// In the absence of the attribute and flag, both ctor and dtor should
+// be emitted, check for that.
+// CHECK: @__cxx_global_var_init
+// CHECK: @__cxa_atexit
+
+// When attribute is enabled, the constructor should not be balanced
+// by a destructor. Make sure we have the ctor but not the dtor
+// registration.
+// CHECK-ATTR: @__cxx_global_var_init
+// CHECK-ATTR-NOT: @__cxa_atexit
+
+// Same scenario except with global flag (-fno-c++-static-destructors)
+// supressing it instead of the attribute. 
+// CHECK-FLAG: @__cxx_global_var_init
+// CHECK-FLAG-NOT: @__cxa_atexit
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -64,6 +64,15 @@
 /// static storage duration.
 static void EmitDeclDestroy(CodeGenFunction &CGF, const VarDecl &D,
 ConstantAddress Addr) {
+  // Honor __attribute__((no_destroy)) and bail instead of attempting
+  // to emit a reference to a possibly nonexistent destructor, which
+  // in turn can cause a crash. This will result in a global constructor
+  // that isn't balanced out by a destructor call as intended by the
+  // attribute. This also checks for -fno-c++-static-destructors and
+  // bails even if the attribute is not present.
+  if (D.isNoDestroy(CGF.getContext()))
+return;
+  
   CodeGenModule &CGM = CGF.CGM;
 
   // FIXME:  __attribute__((cleanup)) ?


Index: test/CodeGenCXX/attr-no-destroy-d54344.cpp
===
--- test/CodeGenCXX/attr-no-destroy-d54344.cpp
+++ test/CodeGenCXX/attr-no-destroy-d54344.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu -DNOATTR  %s -o - | FileCheck %s
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu %s -o - | FileCheck %s --check-prefix=CHECK-ATTR
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu -DNOATTR -fno-c++-static-destructors %s -o - | FileCheck %s --check-prefix=CHECK-FLAG
+
+// Regression test for D54344. Class with no user-defined destructor
+// that has an inherited member that has a non-trivial destructor
+// and a non-default constructor will attempt to emit a destructor
+// despite being marked as __attribute((no_destroy)) in which case
+// it would trigger an assertion due to an incorrect assumption.
+// This test requires asserts to reliably work as without the crash
+// it generates working but semantically incorrect code.
+
+class a {
+public:
+  ~a();
+};
+class logger_base {
+  a d;
+};
+class e : logger_base {};
+#ifndef NOATTR
+__attribute((no_destroy))
+#endif
+e g;
+
+// In the absence of the attribute and flag, both ctor and dtor should
+// be emitted, check for that.
+// CHECK: @__cxx_global_var_init
+// CHECK: @__cxa_atexit
+
+// When attribute is enabled, the constructor should not be balanced
+// by a destructor. Make sure we have the ctor but not the dtor
+// registration.
+// CHECK-ATTR: @__cxx_global_var_init
+// CHECK-ATTR-NOT: @__cxa_atexit
+
+// Same scenario except with global flag (-fno-c++-static-destructors)
+// supressing it instead of the attribute. 
+// CHECK-FLAG: @__cxx_global_var_init
+// CHECK-FLAG-NOT: @__cxa_atexit
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -64,6 +64,15 @@
 /// static storage d

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Though on the other hand it makes no sense to skip it with asserts off either, 
that just clicked in my head, sorry.


Repository:
  rC Clang

https://reviews.llvm.org/D54344



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

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

[Kristina Brooks via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346628: [CodeGen][CXX]: Fix no_destroy CG bug under specific 
circumstances (authored by kristina, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54344?vs=173602&id=173605#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D54344

Files:
  cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
  cfe/trunk/test/CodeGenCXX/attr-no-destroy-d54344.cpp


Index: cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
===
--- cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
+++ cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
@@ -64,6 +64,15 @@
 /// static storage duration.
 static void EmitDeclDestroy(CodeGenFunction &CGF, const VarDecl &D,
 ConstantAddress Addr) {
+  // Honor __attribute__((no_destroy)) and bail instead of attempting
+  // to emit a reference to a possibly nonexistent destructor, which
+  // in turn can cause a crash. This will result in a global constructor
+  // that isn't balanced out by a destructor call as intended by the
+  // attribute. This also checks for -fno-c++-static-destructors and
+  // bails even if the attribute is not present.
+  if (D.isNoDestroy(CGF.getContext()))
+return;
+  
   CodeGenModule &CGM = CGF.CGM;
 
   // FIXME:  __attribute__((cleanup)) ?
Index: cfe/trunk/test/CodeGenCXX/attr-no-destroy-d54344.cpp
===
--- cfe/trunk/test/CodeGenCXX/attr-no-destroy-d54344.cpp
+++ cfe/trunk/test/CodeGenCXX/attr-no-destroy-d54344.cpp
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu 
-DNOATTR  %s -o - | FileCheck %s
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu 
%s -o - | FileCheck %s --check-prefix=CHECK-ATTR
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu 
-DNOATTR -fno-c++-static-destructors %s -o - | FileCheck %s 
--check-prefix=CHECK-FLAG
+
+// Regression test for D54344. Class with no user-defined destructor
+// that has an inherited member that has a non-trivial destructor
+// and a non-default constructor will attempt to emit a destructor
+// despite being marked as __attribute((no_destroy)) in which case
+// it would trigger an assertion due to an incorrect assumption.
+
+// This test is more reliable with asserts to work as without 
+// the crash may (unlikely) could generate working but semantically
+// incorrect code.
+
+class a {
+public:
+  ~a();
+};
+class logger_base {
+  a d;
+};
+class e : logger_base {};
+#ifndef NOATTR
+__attribute((no_destroy))
+#endif
+e g;
+
+// In the absence of the attribute and flag, both ctor and dtor should
+// be emitted, check for that.
+// CHECK: @__cxx_global_var_init
+// CHECK: @__cxa_atexit
+
+// When attribute is enabled, the constructor should not be balanced
+// by a destructor. Make sure we have the ctor but not the dtor
+// registration.
+// CHECK-ATTR: @__cxx_global_var_init
+// CHECK-ATTR-NOT: @__cxa_atexit
+
+// Same scenario except with global flag (-fno-c++-static-destructors)
+// supressing it instead of the attribute. 
+// CHECK-FLAG: @__cxx_global_var_init
+// CHECK-FLAG-NOT: @__cxa_atexit


Index: cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
===
--- cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
+++ cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
@@ -64,6 +64,15 @@
 /// static storage duration.
 static void EmitDeclDestroy(CodeGenFunction &CGF, const VarDecl &D,
 ConstantAddress Addr) {
+  // Honor __attribute__((no_destroy)) and bail instead of attempting
+  // to emit a reference to a possibly nonexistent destructor, which
+  // in turn can cause a crash. This will result in a global constructor
+  // that isn't balanced out by a destructor call as intended by the
+  // attribute. This also checks for -fno-c++-static-destructors and
+  // bails even if the attribute is not present.
+  if (D.isNoDestroy(CGF.getContext()))
+return;
+  
   CodeGenModule &CGM = CGF.CGM;
 
   // FIXME:  __attribute__((cleanup)) ?
Index: cfe/trunk/test/CodeGenCXX/attr-no-destroy-d54344.cpp
===
--- cfe/trunk/test/CodeGenCXX/attr-no-destroy-d54344.cpp
+++ cfe/trunk/test/CodeGenCXX/attr-no-destroy-d54344.cpp
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu -DNOATTR  %s -o - | FileCheck %s
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu %s -o - | FileCheck %s --check-prefix=CHECK-ATTR
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu -DNOATTR -fno-c++-static-destructors %s -o - | FileCheck %s --check-prefix=CHECK-FLAG
+
+// Regression test for D54344. Class with no user-defined destructor
+// that has an inherited member that has a non-trivial destructor
+

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina added reviewers: aaron.ballman, erik.pilkington, rsmith.
kristina added a comment.

Alright, once this is fully reviewed, if accepted, I can land the LLVMSupport 
change and add your new target and close the stack. In the meantime, adding 
some more reviewers who have more experience with Clang CR than me and who can 
hopefully weigh in some opinions. (Sorry about the delays, looks like I'll be 
the one seeing addition of this target through, not landed the LLVMSupport 
change yet since I want to make sure this set of patches is good, otherwise the 
LLVMSupport change on its own holds little weight)


Repository:
  rC Clang

https://reviews.llvm.org/D54379



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Added first batch of comments regarding the patch. Some style, some 
semantics-related.




Comment at: lib/Basic/Targets/OSTargets.h:283
+Builder.defineMacro("__GLIBC__");
+Builder.defineMacro("__ELF__");
+if (Opts.POSIXThreads)

`__MACH__` and `__HURD__` seem appropriate? Apple Mach (Darwin/XNU) uses 
`__MACH__` with `__APPLE__`, Hurd should probably follow a similar convention, 
I don't think there are many places aside from XNU build where `__MACH__` is 
used on its own.



Comment at: lib/Driver/ToolChains/Hurd.cpp:72
+ const ArgList &Args)
+: Generic_ELF(D, Triple, Args) { }
+

No space here.



Comment at: lib/Driver/ToolChains/Hurd.cpp:78
+
+  return std::string();
+}

I'm not quite sure I like this. Also early return should be for the "bad" case, 
not for the good case, at least IMO, this is not a huge issue but I'll see what 
others say. I think this may just be subjective.



Comment at: lib/Driver/ToolChains/Hurd.cpp:84
+  const Driver &D = getDriver();
+  std::string SysRoot = computeSysRoot();
+

Move semantics? Or is this guaranteed elision (I'm not sure)?



Comment at: lib/Driver/ToolChains/Hurd.cpp:118
+  const StringRef X86MultiarchIncludeDirs[] = {
+  "/usr/include/i386-gnu"};
+

Until needed I wouldn't use an array here, also drop const.


Repository:
  rC Clang

https://reviews.llvm.org/D54379



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina added inline comments.



Comment at: lib/Driver/Driver.cpp:392
 
+static void replaceString(std::string &S,
+  const char *Other,

Same as previous comment regarding this type of function.


Repository:
  rC Clang

https://reviews.llvm.org/D54379



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Phab lost this inline command for some reason, but please leave some comment 
regarding why that part in `Driver.cpp` does what it does (summarize the 
conclusion of the discussion in https://reviews.llvm.org/D54378).




Comment at: lib/Driver/Driver.cpp:416
+  std::string T = TargetTriple;
+  replaceString(T, "-unknown-gnu", "-unknown-hurd-gnu");
+  replaceString(T, "-pc-gnu", "-pc-hurd-gnu");

Please comment the reasoning behind this (ie. the discussion in D54378) for any 
other maintainers.


Repository:
  rC Clang

https://reviews.llvm.org/D54379



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

The other lost comment was regarding the functions where you're using `strcpy` 
instead of idiomatic LLVM and also creating unnecessary temporary `std::string` 
instances on the stack.


Repository:
  rC Clang

https://reviews.llvm.org/D54379



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Commented on that particular idiom, there's two instances where it's used, 
aside from variable naming issues (`pos` should be `Pos`) it's very non 
idiomatic as far as rest of LLVM code goes, don't pass string literals around 
as `const char*`, prefer `StringRef` instead, that would also save you from 
needing to resort to using `strlen` later (sorry for previous comment, I didn't 
mean `strcpy`).




Comment at: lib/Driver/Driver.cpp:400
+
+  S.replace(pos, strlen(Other), Replace);
+}

Please avoid that kind of code and avoid `strlen`, you should pass things as 
StringRef as that's the general way of doing things unless you have a good 
reason for doing so otherwise. This entire function/part that uses it should be 
rewritten, I especially don't like the temporary `std::string` on the stack. It 
may be worth considering SmallString which is a variation of SmallVector or 
just manipulating the StringRef. You very certainly don't need `strlen` 
however, StringRef provides the needed operators, same goes for using 
`std::string::find`, just use StringRef instead. 


Repository:
  rC Clang

https://reviews.llvm.org/D54379



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina added inline comments.



Comment at: lib/Driver/Driver.cpp:418
+  replaceString(T, "-pc-gnu", "-pc-hurd-gnu");
+  TargetTriple = T;
+

Reference to a local variable?


Repository:
  rC Clang

https://reviews.llvm.org/D54379



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina added inline comments.



Comment at: lib/Driver/Driver.cpp:418
+  replaceString(T, "-pc-gnu", "-pc-hurd-gnu");
+  TargetTriple = T;
+

kristina wrote:
> Reference to a local variable?
Hm, actually this is fine I guess, just avoid `strlen` and pass literals as 
StringRefs. I would make sure the triple actually matches before you construct 
a local though, otherwise you're forcing doing it for every target, which in 
majority of the cases isn't going to be Hurd. I would still use `SmallString` 
instead of `std::string` but that's more of a nitpick.


Repository:
  rC Clang

https://reviews.llvm.org/D54379



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Also, this needs unit tests and FileCheck tests.


Repository:
  rC Clang

https://reviews.llvm.org/D54379



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

As discussed in `#hurd`, a few additional comments. The Hurd codepath should 
first check if the triple is eligible, ie. minimizing any cost to the driver 
invocation for most targets, which are not going to be Hurd. In fact I would 
wrap it in `LLVM_UNLIKELY` but that's just a suggestion. Once you confirmed 
that the triple in question is the one you're looking for (from the suffix), 
you can alter the existing triple. The `std::string` should be avoided here 
since even a zero length `SmallVector` will guarantee not allocating anything 
unless used. This may be worth factoring out into a separate function entirely, 
and another important point is avoiding any sort of unneeded overhead unless 
you've confirmed that it's the Hurd triple.

In general, I've looked over it again and I'd ask to get rid of switches that 
can be replaced with `if` statements. If there's a need later, you can add it 
later. For now, there's no need so I would avoid it. Switch is not generally an 
easy construct for humans to parse. Avoid arrays with 1 element until needed as 
well please. You can always introduce further changes as they're needed, 
however, for now, I'm strongly against adding "clutter" because it may be 
useful in the future.

I'll end this comment with this: In general when structuring your code, the 
performance penalty for other targets when the conditions that can be easily 
tested are not met should pretty much be close to nonexistent. I would suggest 
keeping that in mind before when submitting revisions.


Repository:
  rC Clang

https://reviews.llvm.org/D54379



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

I'll re-review when I'm up, from a quick glance it looks much better but I'll 
have to patch it over my fork and try out a few things (Mostly x86_64 Linux and 
Darwin test suites). I think the test is lacking a bit, there's a lot of stuff 
that isn't covered, and there's a lack of negative tests (ie. when invalid 
input is supplied and matches the triple suffix). Feel free to run tests 
though, may find something before me, I'm a bit too tired to reconfigure my 
buildbot to do what I want right now, so I'll leave it until when I'm up. So 
yeah I may be being a bit nitpicky but I think tests could cover a little bit 
more.

It would also be great to get at least one other Clang reviewer to sign off on 
this. I can sign off on this myself once I test it, but I feel like getting 
another set of eyes would be good. But yeah if this is all good and someone 
else can also skim through this and sign off on it, I can commit the stack when 
I'm up and when I've done some stuff. If you can, try to build/test with a 
recent Clang as that usually brings out some benign warnings one may miss if 
using an older SDK.

But very good job in general, this seems a lot better and streamlined than the 
previous revision.

So that said to other Clang reviewers: Gentle ping, would really love a second 
set of eyes though, though it can wait until Monday at worst since I'd imagine 
a lot of people are off right now.

Thank you.


Repository:
  rC Clang

https://reviews.llvm.org/D54379



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

[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a reviewer: kristina.
kristina added a comment.

Do you mind resubmitting those with context? Also I would suggest asking Tom 
Stellard as he's in charge of handling cherrypicking patches to go into 
releases once the major rolls over and I think we're pretty close (?) to 7.0.1.


Repository:
  rC Clang

https://reviews.llvm.org/D53696



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

[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Linux, Solaris and OpenBSD also define it where appropriate, it depends on the 
target. This is why it's much easier to review when full context (diff with `-U 
9`) is provided, and means that it has a much lower change of `patch` doing 
something unexpected.


Repository:
  rC Clang

https://reviews.llvm.org/D53696



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

[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64

[Kristina Brooks via Phabricator via cfe-commits]

kristina requested changes to this revision.
kristina added a comment.

In https://reviews.llvm.org/D53696#1305990, @kallisti5 wrote:

> Indeed. I agree the _GLIBCXX_USE_FLOAT128 is misplaced there.   That comes 
> from config.h in libstdc++.  That's working around an issue in Haiku's build 
> (libstdc++ is built by gcc, but then we try and use it with clang later)
>  I can remove that.
>
> Is the __FLOAT128__ desireable?


Sorry I was referring to `__FLOAT128__` being standard, the other define 
shouldn't be there IMO. But then again I'm not an expert on Haiku, 
`__FLOAT128__` seems fine to me, the other change looks more questionable, if 
anyone is familiar with Haiku internals, please chime in.


Repository:
  rC Clang

https://reviews.llvm.org/D53696



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Ping. Would like someone else to co-review this, everything looks fine aside 
from tests (I think a portion of this could use a short unit test with regards 
to creating the actual target instance). Also your FileCheck test is only for 
invoking `clang -cc1` from the looks, can you add coverage for static linker 
invocations (as driver is also generally used to invoke the linker, unless 
that's explicitly not the case on Hurd)?


Repository:
  rC Clang

https://reviews.llvm.org/D54379



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

[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Ping. Is the author still around? I'm happy to take over this revision if not, 
and add `__FLOAT128__` but unless someone says `_GLIBCXX_USE_FLOAT128` is 
actually defined by the toolchain, I'll remove that line, it looks like it 
really shouldn't be defined in the toolchain, but I don't have a Haiku 
toolchain around so I can't say for sure.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53696



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Alright, will patch it into my fork in a bit and try it out, if everything 
looks good, I'll land the stack. (Again huge sorry about this taking some time, 
have lots on my hands at the moment)


Repository:
  rC Clang

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

https://reviews.llvm.org/D54379



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

[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64

[Kristina Brooks via Phabricator via cfe-commits]

kristina commandeered this revision.
kristina edited reviewers, added: return; removed: kristina.
kristina added a comment.

New revision in D54901 .


Repository:
  rC Clang

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

https://reviews.llvm.org/D53696



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

[PATCH] D54901: [Haiku] Support __float128 for Haiku x86 and x86_64

[Kristina Brooks via Phabricator via cfe-commits]

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

LGTM. (Revision of D53696 )


Repository:
  rC Clang

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

https://reviews.llvm.org/D54901



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

[PATCH] D54901: [Haiku] Support __float128 for Haiku x86 and x86_64

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Let me know if you need this committed.




Comment at: lib/Basic/Targets/OSTargets.h:260
 DefineStd(Builder, "unix", Opts);
+if (this->HasFloat128) {
+  Builder.defineMacro("__FLOAT128__");

One tiny style-related nitpick, no need for braces here, can just remove them 
after applying the patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54901



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Your tests don't pass:

  
  Testing Time: 21.51s
  
  Failing Tests (1):
  Clang :: Driver/hurd.c
  
Expected Passes: 470
Expected Failures  : 3
Unsupported Tests  : 71
Unexpected Failures: 1

Seems to be an issue with this particular path:

  /q/src/llvm-tainted-8.0/tools/clang/test/Driver/hurd.c:9:11: error: CHECK: 
expected string not found in input
  // CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"




Repository:
  rC Clang

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

https://reviews.llvm.org/D54379



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Hm, yes you're right, the files aren't in the diff that Phab gives me, this is 
annoying. I wonder if it's stripping them from the diff for some reason, can 
you upload the raw diff?


Repository:
  rC Clang

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

https://reviews.llvm.org/D54379



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina accepted this revision as: kristina.
kristina added a comment.

Actually nevermind, I'll just create them myself, seems like a strange bug.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54379



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

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

Yes everything passes now, my bad for not noticing.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54379



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC347833: Add Hurd target to Clang driver (2/2) (authored by 
kristina, committed by ).

Repository:
  rC Clang

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

https://reviews.llvm.org/D54379

Files:
  lib/Basic/Targets.cpp
  lib/Basic/Targets/OSTargets.h
  lib/Driver/CMakeLists.txt
  lib/Driver/Driver.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/Gnu.cpp
  lib/Driver/ToolChains/Hurd.cpp
  lib/Driver/ToolChains/Hurd.h
  lib/Frontend/InitHeaderSearch.cpp
  test/Driver/Inputs/basic_hurd_tree/include/.keep
  test/Driver/Inputs/basic_hurd_tree/lib/i386-gnu/.keep
  test/Driver/Inputs/basic_hurd_tree/lib32/.keep
  test/Driver/Inputs/basic_hurd_tree/usr/include/i386-gnu/.keep
  test/Driver/Inputs/basic_hurd_tree/usr/lib/i386-gnu/.keep
  test/Driver/Inputs/basic_hurd_tree/usr/lib32/.keep
  test/Driver/hurd.c

Index: test/Driver/hurd.c
===
--- test/Driver/hurd.c
+++ test/Driver/hurd.c
@@ -0,0 +1,62 @@
+// RUN: %clang -no-canonical-prefixes %s -### 2>&1 \
+// RUN: --target=i386-pc-gnu \
+// RUN: --sysroot=%S/Inputs/basic_hurd_tree \
+// RUN:   | FileCheck --check-prefix=CHECK %s
+// CHECK-NOT: warning:
+// CHECK: "-cc1"
+// CHECK: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/include"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
+// CHECK: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK: "-dynamic-linker" "/lib/ld.so"
+// CHECK: "crtbegin.o"
+// CHECK: "-L[[SYSROOT]]/lib/i386-gnu"
+// CHECK: "-L[[SYSROOT]]/lib/../lib32"
+// CHECK: "-L[[SYSROOT]]/usr/lib/i386-gnu"
+// CHECK: "-L[[SYSROOT]]/usr/lib/../lib32"
+// CHECK: "-L[[SYSROOT]]/lib"
+// CHECK: "-L[[SYSROOT]]/usr/lib"
+
+// RUN: %clang -no-canonical-prefixes %s -### 2>&1 \
+// RUN: --target=i386-pc-gnu -static \
+// RUN: --sysroot=%S/Inputs/basic_hurd_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-STATIC %s
+// CHECK-STATIC-NOT: warning:
+// CHECK-STATIC: "-cc1"
+// CHECK-STATIC: "-static-define"
+// CHECK-STATIC: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK-STATIC: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
+// CHECK-STATIC: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"
+// CHECK-STATIC: "-internal-externc-isystem" "[[SYSROOT]]/include"
+// CHECK-STATIC: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
+// CHECK-STATIC: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-STATIC: "-static"
+// CHECK-STATIC: "crtbeginT.o"
+// CHECK-STATIC: "-L[[SYSROOT]]/lib/i386-gnu"
+// CHECK-STATIC: "-L[[SYSROOT]]/lib/../lib32"
+// CHECK-STATIC: "-L[[SYSROOT]]/usr/lib/i386-gnu"
+// CHECK-STATIC: "-L[[SYSROOT]]/usr/lib/../lib32"
+// CHECK-STATIC: "-L[[SYSROOT]]/lib"
+// CHECK-STATIC: "-L[[SYSROOT]]/usr/lib"
+
+// RUN: %clang -no-canonical-prefixes %s -### 2>&1 \
+// RUN: --target=i386-pc-gnu -shared \
+// RUN: --sysroot=%S/Inputs/basic_hurd_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-SHARED %s
+// CHECK-SHARED-NOT: warning:
+// CHECK-SHARED: "-cc1"
+// CHECK-SHARED: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK-SHARED: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
+// CHECK-SHARED: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"
+// CHECK-SHARED: "-internal-externc-isystem" "[[SYSROOT]]/include"
+// CHECK-SHARED: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
+// CHECK-SHARED: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-SHARED: "crtbeginS.o"
+// CHECK-SHARED: "-L[[SYSROOT]]/lib/i386-gnu"
+// CHECK-SHARED: "-L[[SYSROOT]]/lib/../lib32"
+// CHECK-SHARED: "-L[[SYSROOT]]/usr/lib/i386-gnu"
+// CHECK-SHARED: "-L[[SYSROOT]]/usr/lib/../lib32"
+// CHECK-SHARED: "-L[[SYSROOT]]/lib"
+// CHECK-SHARED: "-L[[SYSROOT]]/usr/lib"
Index: lib/Driver/ToolChains/Hurd.cpp
===
--- lib/Driver/ToolChains/Hurd.cpp
+++ lib/Driver/ToolChains/Hurd.cpp
@@ -0,0 +1,169 @@
+//===--- Hurd.cpp - Hurd ToolChain Implementations *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Hurd.h"
+#include "CommonArgs.h"
+#include "clang/Config/config.h"
+#include "clang/Driver/Driver.h"
+#include "clang/Driver/Options.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/VirtualFileSystem.h"
+
+using namespace clang::driver;
+using namespace clang::driver::toolchains;
+using namespace clang;
+using namespace llvm::opt;
+
+using tools::addPathIfExists;
+
+/// Get our best guess at the multiarch triple for a

[PATCH] D54901: [Haiku] Support __float128 for Haiku x86 and x86_64

[Kristina Brooks via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348368: [Haiku] Support __float128 for x86 and x86_64 
(authored by kristina, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54901?vs=175266&id=176815#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D54901

Files:
  cfe/trunk/lib/Basic/Targets/OSTargets.h
  cfe/trunk/test/CodeGenCXX/float128-declarations.cpp


Index: cfe/trunk/test/CodeGenCXX/float128-declarations.cpp
===
--- cfe/trunk/test/CodeGenCXX/float128-declarations.cpp
+++ cfe/trunk/test/CodeGenCXX/float128-declarations.cpp
@@ -14,6 +14,10 @@
 // RUN:   %s -o - | FileCheck %s -check-prefix=CHECK-X86
 // RUN: %clang_cc1 -emit-llvm -triple x86_64-pc-solaris2.11 -std=c++11 \
 // RUN:   %s -o - | FileCheck %s -check-prefix=CHECK-X86
+// RUN: %clang_cc1 -emit-llvm -triple i586-pc-haiku -std=c++11 \
+// RUN:   %s -o - | FileCheck %s -check-prefix=CHECK-X86
+// RUN: %clang_cc1 -emit-llvm -triple x86_64-unknown-haiku -std=c++11 \
+// RUN:   %s -o - | FileCheck %s -check-prefix=CHECK-X86
 //
 /*  Various contexts where type __float128 can appear. The different check
 prefixes are due to different mangling on X86.  */
Index: cfe/trunk/lib/Basic/Targets/OSTargets.h
===
--- cfe/trunk/lib/Basic/Targets/OSTargets.h
+++ cfe/trunk/lib/Basic/Targets/OSTargets.h
@@ -257,6 +257,8 @@
 Builder.defineMacro("__HAIKU__");
 Builder.defineMacro("__ELF__");
 DefineStd(Builder, "unix", Opts);
+if (this->HasFloat128) 
+  Builder.defineMacro("__FLOAT128__");
   }
 
 public:
@@ -267,6 +269,14 @@
 this->PtrDiffType = TargetInfo::SignedLong;
 this->ProcessIDType = TargetInfo::SignedLong;
 this->TLSSupported = false;
+switch (Triple.getArch()) {
+default:
+  break;
+case llvm::Triple::x86:
+case llvm::Triple::x86_64:
+  this->HasFloat128 = true;
+  break;
+}
   }
 };
 


Index: cfe/trunk/test/CodeGenCXX/float128-declarations.cpp
===
--- cfe/trunk/test/CodeGenCXX/float128-declarations.cpp
+++ cfe/trunk/test/CodeGenCXX/float128-declarations.cpp
@@ -14,6 +14,10 @@
 // RUN:   %s -o - | FileCheck %s -check-prefix=CHECK-X86
 // RUN: %clang_cc1 -emit-llvm -triple x86_64-pc-solaris2.11 -std=c++11 \
 // RUN:   %s -o - | FileCheck %s -check-prefix=CHECK-X86
+// RUN: %clang_cc1 -emit-llvm -triple i586-pc-haiku -std=c++11 \
+// RUN:   %s -o - | FileCheck %s -check-prefix=CHECK-X86
+// RUN: %clang_cc1 -emit-llvm -triple x86_64-unknown-haiku -std=c++11 \
+// RUN:   %s -o - | FileCheck %s -check-prefix=CHECK-X86
 //
 /*  Various contexts where type __float128 can appear. The different check
 prefixes are due to different mangling on X86.  */
Index: cfe/trunk/lib/Basic/Targets/OSTargets.h
===
--- cfe/trunk/lib/Basic/Targets/OSTargets.h
+++ cfe/trunk/lib/Basic/Targets/OSTargets.h
@@ -257,6 +257,8 @@
 Builder.defineMacro("__HAIKU__");
 Builder.defineMacro("__ELF__");
 DefineStd(Builder, "unix", Opts);
+if (this->HasFloat128) 
+  Builder.defineMacro("__FLOAT128__");
   }
 
 public:
@@ -267,6 +269,14 @@
 this->PtrDiffType = TargetInfo::SignedLong;
 this->ProcessIDType = TargetInfo::SignedLong;
 this->TLSSupported = false;
+switch (Triple.getArch()) {
+default:
+  break;
+case llvm::Triple::x86:
+case llvm::Triple::x86_64:
+  this->HasFloat128 = true;
+  break;
+}
   }
 };
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Personally I'm against this type of warning as it's likely anyone using 
`-mllvm` is actually intending to adjust certain behaviors across one or more 
passes with a lot of switches supported by it being intentionally hidden from 
` --help` output requiring explicitly specifying that hidden flags 
be shown. One real use case being a dozen of patches to my downstream 
LLVM/Clang fork, for example *very* experimental support for SEH on Itanium 
ABI. I feel like this is going to affect developers more than users as now 
additional flags will have to be passed to suppress the warnings generated from 
using flags to debug/diagnose passes by code authors themselves. I feel like 
using `-mllvm` already implies explicit understanding of that, and of the 
`cl::opt` semantics/purpose as well as the flags being generally out of public 
view unless one gets them from full help (which explicitly shows all registered 
flags, hidden or not), or from source code which is most likely to be the 
developers themselves.

For example, I routinely use the following with SEH (excuse some of the naming, 
this is just a downstream fork however):

  -mllvm -target-enable-seh=true -mllvm -force-msvc-seh=true -mllvm 
-wtfabi-opts=0x1EF77F

I like the ability to pass those via the driver seamlessly, other options being 
explicitly constructing a `clang -cc1` call myself, which is very verbose and 
the driver helps with that a huge amount or adding `#if 0` around them 
downstream (better than commenting out since it's unlikely to cause merge 
conflicts).

I'm mostly indifferent towards `-Xclang` however (since I very rarely used it, 
I think `-Xclang -fexternc-nounwind` is the only time I used it, so I don't 
really have a strong opinion regarding that diagnostic, I still think it's not 
a good change. (speaking of, I should probably make a diff to expose that to 
the frontend via driver as it was seemingly missed in compiler invocation 
argument building, from its `-f` prefix I'm guessing that was accidental but I 
haven't looked into it, which I definitely should)).

If I may suggest another option, is it possible to add a "maintainer mode" flag 
to the build system (ie. with CMake, `-DLLVM_MAINTAINER_MODE=ON`, and a similar 
style thing with GN) and guard the diagnostic emission with `#ifndef 
LLVM_MAINTAINER_MODE`. This would easily allow developers to experiement with 
LLVM downstream without needing explicit workarounds for supressing those 
warnings. I would be happy to write a patch for CMake based builds myself (not 
GN unfortunately, slightly rusty on it) if you feel that is a better 
compromise. This means that downstream developers, whether intending to 
upstream such changes or not, can pass this through and not worry about those 
warnings since this is an explicit "here be dragons" opt-in, as that is easy to 
add to a build system (this also ties in with the previous discussion of adding 
an unsupressable warning for a certain -Xclang flag with the intent of getting 
users to avoid it in releases and yet allow performance data collection, but to 
developers that is more of an annoyance). I feel like this would be the 
ultimate consent to "yes I really want this, I'm aware of what it does" and 
would also require full rebuilds to enable, which is easy if you're developing 
a pass, for example, since you would be rebuilding anyway (assuming in good 
faith that this is only enabled for development builds, by downstream forks or 
build system configurations and releases never have it set). In case of CMake a 
warning may be a good idea as well when that flag is enabled as well as clear 
updates to documentation to reflect the purpose of it.

Anyway that's my opinion/concern on the matter, I don't know if others share it 
or not and I'm not sure if there are glaring problems with the idea of a 
solution I proposed, but I think it's better than some downstream vendors 
excluding this patch altogether and various other (inconsistent) ways 
developers will get around it.

Thanks.


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

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

> There is a cost to having people encode these flags into their build systems 
> -- it can then cause issues if we ever change these internal flags. I do not 
> think any Clang maintainer intends to support these as stable APIs, unlike 
> most of the driver command-line. But, neither -Xclang nor -mllvm obviously 
> scream out "don't use this!", and so people may then add them to their 
> buildsystems without causing reviewers to say "Wait...really? Are you sure 
> that's a good idea?".

This is why I proposed a compromise, allow this warning to be disabled 
completely for people actively using those flags, which are pretty much 
exclusively toolchain developers (well basically what I proposed, since it's 
not clear what counts as a build made by someone who is working and debugging a 
pass, being fully aware of those flags, using the subset of them specific to 
that pass/feature, I would say assertion+dump enabled builds are closest, but 
having an explicit build flag for it would be nicer). It's more unified than 
everyone either adding workarounds into build systems or disabling it 
completely (by just removing it).

Alternatively just let people shoot themselves in the foot, a documentation 
change regarding the dangers of the flag should suffice. Besides, I don't think 
this really ever surfaced as an issue, until Chandler's idea with regards to an 
unsupressable warning for performance tests for 7.x.x (which is a very specific 
and narrow edge case to allow people to collect performance data). Outside of 
that very specific case, have we really had many issues with consumers 
accidentally setting weird flags that they would have to discover in the first 
place. I don't have a strong opinion on `-Xclang` but `-mllvm ` is 
verbose enough to use as is. Maybe it should be made a lot more obvious in one 
way or another but a warning by default seems like it's taking rather drastic 
preemptive measures against a non-issue (do correct me if I'm wrong here).

I do agree that the documentation should definitely scream that it's not stable 
and it's intended for maintainers since it's a convinient interface (the 
`-mllvm` one) for passing flags through the driver all the way to things like 
the MC layer, where needed, and yes these flags can be removed without notice, 
but again, I don't think it's our responsibility to protect users from using 
*intentionally* hidden flags aside from documenting the reason for why they're 
intentionally hidden in a more obvious way, I think the fact that they are 
intentionally not shown in standard LLVM help and yet are available contradicts 
the idea behind this patch, the whole purpose of the flag is to directly 
interact with specific internal parts of LLVM which in itself is not really 
intended to be a stable interface.


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

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

In D55150#1321734 , @thakis wrote:

> I don't have an opinion on this patch (if you force me to have one, I'm 
> weakly in favor), but I agree with the general sentiment. When I told people 
> to not use mllvm and Xclang before, they told me "but if I'm not supposed to 
> use them, why are they advertised in --help"?
>
>   thakis@thakis:~/src/llvm-build$ bin/clang --help | grep Xclang
> -XclangPass  to the clang compiler
>   thakis@thakis:~/src/llvm-build$ bin/clang --help | grep mllvm
> -mllvm   Additional arguments to forward to LLVM's option 
> processing
>
>
> Which to me is a valid point! Maybe we should remove them from --help or say 
> "internal only, don't usually use" there?


I think the documentation for `-mllvm` could definitely emphasize the aspect 
that these flags are not a part of any public or stable interface and are used 
to change internal behavior of LLVM components a lot more. Definitely in favor 
of doing that.

In D55150#1321724 , @jyknight wrote:

> In D55150#1321705 , @kristina wrote:
>
> > > There is a cost to having people encode these flags into their build 
> > > systems -- it can then cause issues if we ever change these internal 
> > > flags. I do not think any Clang maintainer intends to support these as 
> > > stable APIs, unlike most of the driver command-line. But, neither -Xclang 
> > > nor -mllvm obviously scream out "don't use this!", and so people may then 
> > > add them to their buildsystems without causing reviewers to say 
> > > "Wait...really? Are you sure that's a good idea?".
> >
> > This is why I proposed a compromise, allow this warning to be disabled 
> > completely for people actively using those flags, which are pretty much 
> > exclusively toolchain developers (well basically what I proposed, since 
> > it's not clear what counts as a build made by someone who is working and 
> > debugging a pass, being fully aware of those flags, using the subset of 
> > them specific to that pass/feature, I would say assertion+dump enabled 
> > builds are closest, but having an explicit build flag for it would be 
> > nicer). It's more unified than everyone either adding workarounds into 
> > build systems or disabling it completely (by just removing it).
>
>
> I mean, I'm not much opposed to adding that -- just that any new build-time 
> options is always a minor shame. But you didn't respond to the other part of 
> my message -- is adding `-Wno-experimental-driver-option` to your 
> compile-flags not already a completely sufficient solution for your use-case?
>
> > Besides, I don't think this really ever surfaced as an issue, until 
> > Chandler's idea with regards to an unsupressable warning for performance 
> > tests for 7.x.x
>
> The primary impetus for me was actually the discovery that Boost's build 
> system was using "-Xclang -include-pth" a few weeks back.


It would be an inconvenience to developers and a lot of patches grow from 
downstream, given that some people may want to disable the flag for 
development, it would basically mean that some may end up with varying 
workarounds as opposed to a uniform one. I don't think that happening would be 
of benefit to anyone. And yes I could add an extra flag for that configuration 
in the build system, in my case, but I don't think I'll be the only one doing 
that so again, this would just cause more fragmentation. I don't see that as a 
good thing.


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

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Kristina Brooks via Phabricator via cfe-commits]

kristina requested changes to this revision.
kristina added a comment.

In D55150#1321829 , @efriedma wrote:

> I'm not sure that putting a warning that can be disabled really helps here; 
> anyone who needs the option will just disable the warning anyway, and then 
> users adding additional options somewhere else in the build system will miss 
> the warning.
>
> Instead, it would probably be better to rename Xclang and mllvm to something 
> that makes it clear the user is doing something unsupported. Maybe 
> "--unstable-llvm-option" and "--unstable-clang-option" or something like 
> that.  (This will lead to some breakage, but the breakage is roughly 
> equivalent for anyone using -Werror.)


Thinking about it more, downstream forks with custom passes may utilize those 
flags in tests, renaming them is definitely not the way to go, that is going to 
cause a lot of problem and possibly a lot of angry downstream users as well as 
contributors. Some out-of-tree test suites will treat warnings as failures so 
that behavior by default is also a possible cause for concern. I *really* think 
just changing the documentation to inform consumers about what the flags are 
intended for. In fact `-mllvm` is used extensively in a lot of lit/FileCheck 
tests, so that's also going to cause problems.

I think it's best to just document these options better, I agree, the 
documentation is extremely poor but anything beyond that will/could cause 
issues in so many places.


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

https://reviews.llvm.org/D55150



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

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

If the author is still missing at the end of next week, any objections to me 
resubmitting a similar patch that just implements `__FILE_NAME__` or 
`__BASE_NAME__` (Need a few more opinions here I guess, personally I think 
`__FILE_NAME__` makes more sense)?

I'll carve it out from my PP extension which simply looks for the last path 
separator (depending on the OS) and only renders the filename after it (or the 
whole path if there's no separator). No need for additional complications like 
depths etc. Since this idea was shot down last time, is it possible to get a 
few people to voice their opinion before I mark this as abandoned and carve out 
and clean up this from my PP extension and add proper tests for it?

Would be appreciated, as this sort of thing is very useful (IMO) so would like 
to know if anyone is really against this proposal.


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

https://reviews.llvm.org/D17741



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

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

[Kristina Brooks via Phabricator via cfe-commits]

kristina commandeered this revision.
kristina added a reviewer: weimingz.
kristina added a comment.

Sorry, forgot about this, will make a new diff with just the macro for review 
later tonight.


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

https://reviews.llvm.org/D17741



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

[PATCH] D61756: Add a __FILE_NAME__ macro.

[Kristina Brooks via Phabricator via cfe-commits]

kristina created this revision.
kristina added reviewers: aaron.ballman, rsmith, rnk.
Herald added a project: clang.

A much simplified version of D17741  which 
adds a new builtin macro `__FILE_NAME__` that is similar to `__FILE__` but only 
renders the last path component. It seems that this functionality is highly 
desired from the consensus on D17741  as a 
separate macro versus a much more complicated and niche patch originally 
proposed in that differential.


Repository:
  rC Clang

https://reviews.llvm.org/D61756

Files:
  include/clang/Lex/Preprocessor.h
  lib/Lex/PPMacroExpansion.cpp
  test/Preprocessor/Inputs/include-subdir/file_name_macro_include.h
  test/Preprocessor/Inputs/include-subdir/h
  test/Preprocessor/file_name_macro.c

Index: test/Preprocessor/file_name_macro.c
===
--- test/Preprocessor/file_name_macro.c
+++ test/Preprocessor/file_name_macro.c
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -E %s -I%S/Inputs | FileCheck -strict-whitespace %s
+// RUN: %clang_cc1 -fms-compatibility -DMS -E %s -I%S/Inputs | FileCheck -check-prefix=CHECK-MS -strict-whitespace %s 
+// RUN: %clang_cc1 -E %s -I%S/Inputs -DBADINC -verify
+
+#ifdef BADINC
+
+// Paranoia.
+
+__FILE_NAME__
+#include  // expected-error {{file not found}}
+__FILE_NAME__
+
+#else
+
+// Reference.
+1: "file_name_macro.c"
+
+
+// Ensure it expands correctly for this file.
+2: __FILE_NAME__
+
+// CHECK: {{^}}1: "file_name_macro.c"
+// CHECK: {{^}}2: "file_name_macro.c"
+
+// Test if inclusion works right.
+#ifdef MS
+#include 
+#else
+#include 
+#endif
+
+#include 
+
+
+// CHECK: {{^}}3: "file_name_macro_include.h"
+// CHECK: {{^}}4: "file_name_macro_include.h"
+// CHECK-NOT: {{^}}5: "file_name_macro_include.h"
+// CHECK-MS: {{^}}5: "file_name_macro_include.h"
+// CHECK: {{^}}6: "h"
+
+#endif
Index: test/Preprocessor/Inputs/include-subdir/h
===
--- test/Preprocessor/Inputs/include-subdir/h
+++ test/Preprocessor/Inputs/include-subdir/h
@@ -0,0 +1 @@
+6: __FILE_NAME__
\ No newline at end of file
Index: test/Preprocessor/Inputs/include-subdir/file_name_macro_include.h
===
--- test/Preprocessor/Inputs/include-subdir/file_name_macro_include.h
+++ test/Preprocessor/Inputs/include-subdir/file_name_macro_include.h
@@ -0,0 +1,6 @@
+3: __FILE_NAME__
+4: "file_name_macro_include.h"
+#ifdef MS
+// Should be the same even when included with backslash.
+5: __FILE_NAME__
+#endif
Index: lib/Lex/PPMacroExpansion.cpp
===
--- lib/Lex/PPMacroExpansion.cpp
+++ lib/Lex/PPMacroExpansion.cpp
@@ -363,6 +363,7 @@
   }
 
   // Clang Extensions.
+  Ident__FILE_NAME__  = RegisterBuiltinMacro(*this, "__FILE_NAME__");
   Ident__has_feature  = RegisterBuiltinMacro(*this, "__has_feature");
   Ident__has_extension= RegisterBuiltinMacro(*this, "__has_extension");
   Ident__has_builtin  = RegisterBuiltinMacro(*this, "__has_builtin");
@@ -1474,7 +1475,8 @@
 // __LINE__ expands to a simple numeric value.
 OS << (PLoc.isValid()? PLoc.getLine() : 1);
 Tok.setKind(tok::numeric_constant);
-  } else if (II == Ident__FILE__ || II == Ident__BASE_FILE__) {
+  else if (II == Ident__FILE__ || II == Ident__BASE_FILE__ ||
+   II == Ident__FILE_NAME__) {
 // C99 6.10.8: "__FILE__: The presumed name of the current source file (a
 // character string literal)". This can be affected by #line.
 PresumedLoc PLoc = SourceMgr.getPresumedLoc(Tok.getLocation());
@@ -1495,7 +1497,25 @@
 // Escape this filename.  Turn '\' -> '\\' '"' -> '\"'
 SmallString<128> FN;
 if (PLoc.isValid()) {
-  FN += PLoc.getFilename();
+  // __FILE_NAME__ is a Clang-specific extension that expands to the
+  // the last part of __FILE__.
+  if (II == Ident__FILE_NAME__) {
+// Try to get the last path component.
+StringRef PLFileName = PLoc.getFilename(); 
+size_t LastSep = PLFileName.find_last_of('/');
+// With MS extensions, '\' is a valid path separator, try it as well.
+if (LastSep == StringRef::npos && LangOpts.MicrosoftExt)
+  LastSep = PLFileName.find_last_of('\\');
+// Skip the separator and get the last part, otherwise fall back on
+// returning the original full filename.
+if (LastSep != StringRef::npos) {
+  FN += PLFileName.substr(LastSep+1);
+} else {
+  FN += PLFileName;
+}
+  } else {
+FN += PLoc.getFilename();
+  }
   Lexer::Stringify(FN);
   OS << '"' << FN << '"';
 }
Index: include/clang/Lex/Preprocessor.h
===
--- include/clang/Lex/Preprocessor.h
+++ include/clang/Lex/Preprocessor.h
@@ -147,6 +147,7 @@
   IdentifierInfo *Ident__

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

[Kristina Brooks via Phabricator via cfe-commits]

kristina abandoned this revision.
kristina added a comment.

Superseded by D61756 


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

https://reviews.llvm.org/D17741



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

[PATCH] D61756: Add a __FILE_NAME__ macro.

[Kristina Brooks via Phabricator via cfe-commits]

kristina updated this revision to Diff 198894.
kristina added a comment.

Fix style, remove unnecessary braces, add missing newline.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61756

Files:
  include/clang/Lex/Preprocessor.h
  lib/Lex/PPMacroExpansion.cpp
  test/Preprocessor/Inputs/include-subdir/file_name_macro_include.h
  test/Preprocessor/Inputs/include-subdir/h
  test/Preprocessor/file_name_macro.c

Index: test/Preprocessor/file_name_macro.c
===
--- test/Preprocessor/file_name_macro.c
+++ test/Preprocessor/file_name_macro.c
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -E %s -I%S/Inputs | FileCheck -strict-whitespace %s
+// RUN: %clang_cc1 -fms-compatibility -DMS -E %s -I%S/Inputs | FileCheck -check-prefix=CHECK-MS -strict-whitespace %s 
+// RUN: %clang_cc1 -E %s -I%S/Inputs -DBADINC -verify
+
+#ifdef BADINC
+
+// Paranoia.
+
+__FILE_NAME__
+#include  // expected-error {{file not found}}
+__FILE_NAME__
+
+#else
+
+// Reference.
+1: "file_name_macro.c"
+
+
+// Ensure it expands correctly for this file.
+2: __FILE_NAME__
+
+// CHECK: {{^}}1: "file_name_macro.c"
+// CHECK: {{^}}2: "file_name_macro.c"
+
+// Test if inclusion works right.
+#ifdef MS
+#include 
+#else
+#include 
+#endif
+
+#include 
+
+
+// CHECK: {{^}}3: "file_name_macro_include.h"
+// CHECK: {{^}}4: "file_name_macro_include.h"
+// CHECK-NOT: {{^}}5: "file_name_macro_include.h"
+// CHECK-MS: {{^}}5: "file_name_macro_include.h"
+// CHECK: {{^}}6: "h"
+
+#endif
Index: test/Preprocessor/Inputs/include-subdir/h
===
--- test/Preprocessor/Inputs/include-subdir/h
+++ test/Preprocessor/Inputs/include-subdir/h
@@ -0,0 +1 @@
+6: __FILE_NAME__
Index: test/Preprocessor/Inputs/include-subdir/file_name_macro_include.h
===
--- test/Preprocessor/Inputs/include-subdir/file_name_macro_include.h
+++ test/Preprocessor/Inputs/include-subdir/file_name_macro_include.h
@@ -0,0 +1,6 @@
+3: __FILE_NAME__
+4: "file_name_macro_include.h"
+#ifdef MS
+// Should be the same even when included with backslash.
+5: __FILE_NAME__
+#endif
Index: lib/Lex/PPMacroExpansion.cpp
===
--- lib/Lex/PPMacroExpansion.cpp
+++ lib/Lex/PPMacroExpansion.cpp
@@ -363,6 +363,7 @@
   }
 
   // Clang Extensions.
+  Ident__FILE_NAME__  = RegisterBuiltinMacro(*this, "__FILE_NAME__");
   Ident__has_feature  = RegisterBuiltinMacro(*this, "__has_feature");
   Ident__has_extension= RegisterBuiltinMacro(*this, "__has_extension");
   Ident__has_builtin  = RegisterBuiltinMacro(*this, "__has_builtin");
@@ -1474,7 +1475,8 @@
 // __LINE__ expands to a simple numeric value.
 OS << (PLoc.isValid()? PLoc.getLine() : 1);
 Tok.setKind(tok::numeric_constant);
-  } else if (II == Ident__FILE__ || II == Ident__BASE_FILE__) {
+  else if (II == Ident__FILE__ || II == Ident__BASE_FILE__ ||
+   II == Ident__FILE_NAME__) {
 // C99 6.10.8: "__FILE__: The presumed name of the current source file (a
 // character string literal)". This can be affected by #line.
 PresumedLoc PLoc = SourceMgr.getPresumedLoc(Tok.getLocation());
@@ -1495,7 +1497,26 @@
 // Escape this filename.  Turn '\' -> '\\' '"' -> '\"'
 SmallString<128> FN;
 if (PLoc.isValid()) {
-  FN += PLoc.getFilename();
+  // __FILE_NAME__ is a Clang-specific extension that expands to the
+  // the last part of __FILE__.
+  if (II == Ident__FILE_NAME__) {
+// Try to get the last path component.
+StringRef PLFileName = PLoc.getFilename(); 
+size_t LastSep = PLFileName.find_last_of('/');
+
+// With MS extensions, '\' is a valid path separator, try it as well.
+if (LastSep == StringRef::npos && LangOpts.MicrosoftExt)
+  LastSep = PLFileName.find_last_of('\\');
+
+// Skip the separator and get the last part, otherwise fall back on
+// returning the original full filename.
+if (LastSep != StringRef::npos)
+  FN += PLFileName.substr(LastSep+1);
+else
+  FN += PLFileName;
+  } else {
+FN += PLoc.getFilename();
+  }
   Lexer::Stringify(FN);
   OS << '"' << FN << '"';
 }
Index: include/clang/Lex/Preprocessor.h
===
--- include/clang/Lex/Preprocessor.h
+++ include/clang/Lex/Preprocessor.h
@@ -147,6 +147,7 @@
   IdentifierInfo *Ident__DATE__, *Ident__TIME__;   // __DATE__, __TIME__
   IdentifierInfo *Ident__INCLUDE_LEVEL__;  // __INCLUDE_LEVEL__
   IdentifierInfo *Ident__BASE_FILE__;  // __BASE_FILE__
+  IdentifierInfo *Ident__FILE_NAME__;  // __FILE_NAME__
   IdentifierInfo *Ident__TIMESTAMP__;  // __TIMESTAMP__
   IdentifierInfo *Ident__COUNTER

[PATCH] D52093: [ToolChains] Linux.cpp: Use the same style across all all multi-arch includes. NFC.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Yeah but what about the rest of them, most are completely on their own line, 
some no newlines, mipsel64 was split across two lines while others weren't, and 
switch is meant to have default as the topmost case IIRC. I mean I'm if you 
don't think it's more readable that way I'll abandon it. Though IMHO reading 
wise it's much easier to read as a list down with separators than as a mix of 
various newline/no-newline styles.


Repository:
  rC Clang

https://reviews.llvm.org/D52093



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

[PATCH] D52151: Also manages clang-X as tool for scan-build

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Nitpick: This regex is far too broad to cover the rare use case where you'd be 
invoking clang as `clang-N`. I think something like `clang(?:\-[789])?` would 
be more suitable?


Repository:
  rC Clang

https://reviews.llvm.org/D52151



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

[PATCH] D45741: Python bindings: Fix handling of file bodies with multi-byte characters

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Would you mind re-uploading these patches with full context (with `diff 
-U9`). 3 lines of context around changes makes this very difficult to 
review. Also I would suggest testing for Python version and using appropriate 
semantics (using `encode` in case of Python3). Not entirely sure if the 
bindings are supposed to be compatible with Python3, but I don't see any harm. 
However explicit tests that avoid changing functionality of existing Python2.7 
code would be better in my opinion.


Repository:
  rC Clang

https://reviews.llvm.org/D45741



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

[PATCH] D52151: Also manages clang-X as tool for scan-build

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Just a minor suggestion, I think it would make it more clear as before LLVM 7, 
Clang did not have a version number with the main executable. GCC is slightly 
less consistent with their formats as they usually ship as host compilers with 
various suffixes, but with Clang it can only be `clang`, `clang-7` or 8 (or 9 
in the future) although it's still generally invoked as `clang` unless you have 
multiple installations.


Repository:
  rC Clang

https://reviews.llvm.org/D52151



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